Last Comment Bug 771720 - Miscellaneous gc/Statistics.cpp improvements
: Miscellaneous gc/Statistics.cpp improvements
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
: general
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-07-06 16:44 PDT by Bill McCloskey (:billm)
Modified: 2012-07-19 13:10 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (16.04 KB, patch)
2012-07-06 16:44 PDT, Bill McCloskey (:billm)
terrence.d.cole: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Bill McCloskey (:billm) 2012-07-06 16:44:37 PDT
Created attachment 639849 [details] [diff] [review]

The biggest improvement here is that I discovered that the page fault counts in bug 745034 are actually pretty expensive to collect. Now that we have ~1000 compartments, we do beginPhase and endPhase maybe 15,000 times in a single GC. That means 15,000 calls to getrusage, which isn't terribly fast. In a large browsing session, this was adding tens of milliseconds to the sweep phase of every GC.

Additional changes:

1. Split the discard_code phase into one that happens during marking and one that happens during sweeping. If these aren't separate, it's hard to know how much of the mark phase or the sweep phase are accounted for by sub-phases.

2. Split the mark_other phase into mark_weak, mark_gray, and mark_gray_and_weak. I've noticed that this phase takes a long time if your browser has been open for a while, so we should try to understand which component is slow.

3. Add new sweep_atoms and sweep_tables phases. I was working on figuring out where we're spending time, and these two end up being significant.

4. Add a free_ti_arena phase. This one is also a significant time sink. Note that I had to add the oldAlloc.freeAll() call in order to account for this time. Previously, this was happening in the destructor for oldAlloc, and it's really hard to account for the time taken by a destructor. The semantics of the code should be unchanged, aside from timing.
Comment 1 User image Terrence Cole [:terrence] 2012-07-06 18:43:03 PDT
Comment on attachment 639849 [details] [diff] [review]

Review of attachment 639849 [details] [diff] [review]:

> In a large browsing session, this was adding tens of milliseconds to the sweep phase of every GC.


Wow, good catch.

I'd be interested to know what the overhead of the PRMJ_Now() are as well.  There's this really appalling "optimization" at the front of the windows implementation to "keep from regressing startup time".  There are only 3 calls to PRMJ_Now() while starting up -- gcstats is by far the largest user in the shell now and it would be nice if we could kill half of prmjtime.cpp.
Comment 2 User image Bill McCloskey (:billm) 2012-07-08 10:28:55 PDT

On Linux, PRMJ_Now() is pretty fast. We spend at most a few milliseconds doing it, even with thousands of compartments. I don't know what sort of performance it gets on Windows, though.
Comment 3 User image Bill McCloskey (:billm) 2012-07-08 10:40:30 PDT
Comment on attachment 639849 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 745034
User impact if declined: GCs may needlessly take a long time.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low. It only affects GC data collection, which shouldn't affect browser behavior at all.
String or UUID changes made by this patch: None.
Comment 4 User image Ryan VanderMeulen [:RyanVM] 2012-07-08 17:50:11 PDT
Comment 5 User image Alex Keybl [:akeybl] 2012-07-12 15:32:04 PDT
Comment on attachment 639849 [details] [diff] [review]

[Triage Comment]
Low risk GC perf fix, approved for Aurora 15.
Comment 6 User image Bill McCloskey (:billm) 2012-07-13 12:39:21 PDT

Note You need to log in before you can comment on or make changes to this bug.