Closed
Bug 1057563
Opened 10 years ago
Closed 10 years ago
Don't wait on the background thread between slices
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
INVALID
mozilla35
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-log
|
Details | |
(deleted),
text/x-log
|
Details | |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
All this does is take an uncontested lock, yet Avi has a trace in bug 1057530 (windows 8.1) where this appears to be taking 5.8ms. Avoiding taking the lock twice in this case is trivial, so maybe we can shave 10% off our max pause here.
Avi, could you grab a MOZ_GCTIMER log from the opt win8 binaries in this try run?
https://tbpl.mozilla.org/?tree=Try&rev=0f59b4ad76ed
Comment 1•10 years ago
|
||
I used my own build which I used to generate the previous log - with your patch applied. I think it's better than comparing a try opt build log to my own build.
If you want me to use the try builds, please also do a try push without the patch - and I'll post the logs from both of your builds.
Comment 2•10 years ago
|
||
Adding a log without the patch for reference.
Scenario (same as the one from comment 1):
- Quite clean profile.
- Single tab on http://testufo.com
- Let the browser run for about a minute
Comment 3•10 years ago
|
||
Attachment #8477697 -
Attachment is obsolete: true
Comment 4•10 years ago
|
||
Unless you know of a proper use case or page or benchmark etc where background threads are common for the GC wait, I couldn't easily find one. I ended up with the following sequence:
- Clean profile
- Open the browser and visit http://peacekeeper.futuremark.com/
- wait 20s
- Start the test (about 5 mins to complete)
- Wait another 10s
- Close the browser.
THe log without the patch attached at the previous comment, and this attachment is the log with the patch.
I don't think it shows that the patch worked, as the wait for background thread still appears at the patched version, and it's not quicker.
Attachment #8477673 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Rather, it shows that I didn't read the log carefully enough. Non-incremental reason is "requested". The only other waitBackgroundSweepOrAllocationEnd is in resetIncrementalGC, which we are doing in this case because someone requested a non-incremental GC. That shouldn't be happening.
Assignee | ||
Comment 6•10 years ago
|
||
With the improved logging in place from bug 1068123, it is clear that we are generally taking about 1us in this phase on linux -- not huge, but still startlingly large for what it's doing. Given that we know this can sometimes have catastrophic performance on windows and since it adds a new assertion on the expected thread state, I think it makes sense to take this despite the marginal typical gains.
Attachment #8492226 -
Flags: review?(jcoppeard)
Comment 7•10 years ago
|
||
Comment on attachment 8492226 [details] [diff] [review]
dont_lock_between_incremental_slices-v1.diff
Review of attachment 8492226 [details] [diff] [review]:
-----------------------------------------------------------------
Yes, makes sense.
Attachment #8492226 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee | ||
Comment 10•10 years ago
|
||
Backed out until we can figure out how to handle the background alloc state.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f026869ee548
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•10 years ago
|
||
The waits we're seeing are probably valid. We may want to split up these tasks in the future to be sure, but for now I think we probably can close this.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•