I'm seeing a ton (20000+) small (8 byte) allocations coming mostly from nsUrlClassifierDBServiceWorker::ParseChunkList (although several other functions in this class use very similar patterns). It looks to me like a function that would work with an nsAutoTArray<nsAutoString, n> (where n is some number that covers most uses). You should be able to reimplement nsCStringArray::ParseString on top of that and avoid heap allocations all together.

yeah that was kinda dumb...

Comment on attachment 288187 [details] [diff] [review] get rid of nsCStringArray usage r+, but it looks like you use the string tokenizing a lot. Would it be better to add a single function that parses a string into a nsAutoTArray (i.e., generalize SplitHost)? That would still do more string copies than in the loop where you use nsDependentCSubstring, but it wouldn't be slower than before.

Comment on attachment 288187 [details] [diff] [review] get rid of nsCStringArray usage SplitHost() can take a nsTArray<nsCAutoString> & instead of a nsAutoTArray<nsCAutoString, 5> & just to make you not need that big mess as a param aside from that looks good.

Checking in src/nsUrlClassifierDBService.cpp; /cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v <-- nsUrlClassifierDBService.cpp new revision: 1.36; previous revision: 1.35 done

This patched was backed out during related to bug 403724. While it's backed out, I'm going to work on a new version that should help allocations in that function a bit more.

This patch caches the chunk lists, greatly reducing how often we need to read/parse/write them to and from the database. It requires the transaction patch in 404645 (otherwise we'd have to sync the cache with every chunk, which is how it works now).

Comment on attachment 291088 [details] [diff] [review] cache chunk lists >+ // Cache the list of add/subtract chunks applied to the table, optionally >+ // parsing the add or sub lists. >+ nsresult CacheChunkLists(PRUint32 tableId, >+ PRBool parseAdds, >+ PRBool parseSubs); Can you add to the comment why we're doing this (i.e., saves lots of db activity). >+ PRBool mHaveCachedLists; >+ PRUint32 mCachedListsTable; These don't seem to get initialized anywhere. > nsresult >+nsUrlClassifierDBServiceWorker::CacheChunkLists(PRUint32 tableId, >+ PRBool parseAdds, >+ PRBool parseSubs) >+{ >+ nsresult rv; >+ >+ if (mHaveCachedLists && mCachedListsTable != tableId) { >+ rv = FlushChunkLists(); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ >+ if (!mHaveCachedLists) { >+ rv = GetChunkLists(tableId, mCachedAddsStr, mCachedSubsStr); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ mHaveCachedLists = PR_TRUE; >+ mCachedListsTable = tableId; >+ } >+ >+ if (parseAdds && !mHaveCachedAddChunks) { >+ ParseChunkList(mCachedAddsStr, mCachedAddChunks); >+ mHaveCachedAddChunks = PR_TRUE; >+ } >+ >+ if (parseSubs && !mHaveCachedSubChunks) { >+ ParseChunkList(mCachedSubsStr, mCachedSubChunks); >+ mHaveCachedSubChunks = PR_TRUE; >+ } Should these be if/else-if statements? They seem to be mutually exclusive, but making that explicit seems better.

(In reply to comment #7) > > nsresult > >+nsUrlClassifierDBServiceWorker::CacheChunkLists(PRUint32 tableId, > >+ PRBool parseAdds, > >+ PRBool parseSubs) > >+{ > >+ nsresult rv; > >+ > >+ if (mHaveCachedLists && mCachedListsTable != tableId) { > >+ rv = FlushChunkLists(); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ } > >+ > >+ if (!mHaveCachedLists) { > >+ rv = GetChunkLists(tableId, mCachedAddsStr, mCachedSubsStr); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ > >+ mHaveCachedLists = PR_TRUE; > >+ mCachedListsTable = tableId; > >+ } > >+ > >+ if (parseAdds && !mHaveCachedAddChunks) { > >+ ParseChunkList(mCachedAddsStr, mCachedAddChunks); > >+ mHaveCachedAddChunks = PR_TRUE; > >+ } > >+ > >+ if (parseSubs && !mHaveCachedSubChunks) { > >+ ParseChunkList(mCachedSubsStr, mCachedSubChunks); > >+ mHaveCachedSubChunks = PR_TRUE; > >+ } > > Should these be if/else-if statements? They seem to be mutually exclusive, but > making that explicit seems better. They aren't really mutually exclusive. Current code happens to only parse one at a time, but there's no good reason why they couldn't do both (like if it happened to know that it would be doing both adds and subs in advance).

Updated with tony's comments (except as noted above). Asking for approval - this patch significantly reduces the amount of parsing, allocations, and database IO while updating the url-classifier database.

Comment on attachment 291266 [details] [diff] [review] updated chunk list cache patch Checked in the caching patch. Checking in nsUrlClassifierDBService.cpp; /cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v <-- nsUrlClassifierDBService.cpp new revision: 1.40; previous revision: 1.39 done

should this be marked fixed?

We still doin work here?

Bug 406856 is going to clean up the last of the problem allocations as a side-effect, so we can close this bug once that lands.

406856 is checked in, marking fixed.