1057563 - Don't wait on the background thread between slices

Status: RESOLVED INVALID

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: dont_lock_between_incremental_slices-v0.diff

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

[Avi Halachmi (:avih)]

Attachment: GC log with the patch from comment 0

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

[Avi Halachmi (:avih)]

Attachment: GC log withOUT the patch

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

[Avi Halachmi (:avih)]

Attachment: GC log peacekeeper unpatched

Comment 4

[Avi Halachmi (:avih)]

Attachment: GC log peacekeeper patched

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.

Comment 5

[Terrence Cole [:terrence]]

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.

Comment 6

[Terrence Cole [:terrence]]

Attachment: dont_lock_between_incremental_slices-v1.diff

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.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8492226 [details] [diff] [review]
dont_lock_between_incremental_slices-v1.diff

Review of attachment 8492226 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, makes sense.

Comment 8

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f395d9d894a

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=0f59b4ad76ed

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f395d9d894a

Comment 10

[Terrence Cole [:terrence]]

Backed out until we can figure out how to handle the background alloc state.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f026869ee548

Comment 11

[Terrence Cole [:terrence]]

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.