Closed Bug 403354 Opened 12 years ago Closed 12 years ago

Get rid of nsCStringArray ParseString usage

Categories

(Toolkit :: Safe Browsing, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: pavlov, Assigned: dcamp)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(2 files, 1 obsolete file)

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...
Assignee: nobody → dcamp
Status: NEW → ASSIGNED
Attachment #288187 - Flags: superreview?(pavlov)
Attachment #288187 - Flags: review?(tony)
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.
Attachment #288187 - Flags: review?(tony) → review+
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.
Attachment #288187 - Flags: superreview?(pavlov) → superreview+
Attachment #288187 - Flags: approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
No longer depends on: 403724
Depends on: 403724
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch cache chunk lists (obsolete) — Splinter Review
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).
Attachment #291088 - Flags: review?(tony)
Depends on: 404645
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.
Attachment #291088 - Flags: review?(tony) → review+
(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.
Attachment #291088 - Attachment is obsolete: true
Attachment #291266 - Flags: approval1.9?
Attachment #291266 - Flags: approval1.9? → approval1.9+
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?
Version: unspecified → Trunk
Depends on: 406856
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.