Closed
Bug 1122640
Opened 9 years ago
Closed 9 years ago
Investigate freeing nursery huge slots off main thread
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
5.62 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
2.82 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Hannes noticed that minor GCs sometimes took a long time on MacOS mainly due to Nursery::freeHugeSlots(). This work can be done in the background off main thread.
Comment 1•9 years ago
|
||
+1. That would benefit Browsermark 2.1 knockout definitely. We take 500ms under linux, 1000ms under macosx (on faster system). So that would remove the extra 500ms that MacOSX needs.
Blocks: 1118938
Assignee | ||
Comment 2•9 years ago
|
||
Add a GCParallelTask to free the huge slots off main thread.
Assignee: nobody → jcoppeard
Attachment #8558456 -
Flags: review?(terrence)
Assignee | ||
Comment 3•9 years ago
|
||
...and update to handle the case where we don't have any threads to use for this.
Attachment #8558456 -
Attachment is obsolete: true
Attachment #8558456 -
Flags: review?(terrence)
Attachment #8558549 -
Flags: review?(terrence)
Comment 4•9 years ago
|
||
Comment on attachment 8558549 [details] [diff] [review] bug1122640-free-huge-slots-off-main-thread v2 Review of attachment 8558549 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/gc/Nursery.cpp @@ +1015,5 @@ > + started = freeHugeSlotsTask->startWithLockHeld(); > + } > + > + if (!started) > + freeHugeSlotsTask->runFromMainThread(runtime()); I'm starting to think that we need to tweak the design of GCParallelTask a bit so that start will implicitly run on the main thread if starting on the background thread fails. Not for this patch though.
Attachment #8558549 -
Flags: review?(terrence) → review+
Comment 5•9 years ago
|
||
Also, please make onOutOfMallocMemory join on this task before returning.
Assignee | ||
Comment 6•9 years ago
|
||
I udpated onOutOfMallocMemory, and also added a destructor to GCParallelTask that called join(), just to be safe. https://hg.mozilla.org/integration/mozilla-inbound/rev/8c11a58bf243
Assignee | ||
Comment 7•9 years ago
|
||
Fixed busage due to missing explicit on single argument constructor: https://hg.mozilla.org/integration/mozilla-inbound/rev/52a98fafd551
Assignee | ||
Comment 8•9 years ago
|
||
And backed out for testGCAllocator jsapi test failures on linux 32 bit. https://hg.mozilla.org/integration/mozilla-inbound/rev/0203370cd4db
Assignee | ||
Comment 9•9 years ago
|
||
I think the issue is probably a timing one with memory being released in the background after the start of this test. Either way, we need to wait for huge slot frees to finish in AutoFinishGC.
Attachment #8559277 -
Flags: review?(terrence)
Comment 10•9 years ago
|
||
Comment on attachment 8559277 [details] [diff] [review] finish-gc-before-alloc-test Review of attachment 8559277 [details] [diff] [review]: ----------------------------------------------------------------- Ouch.
Attachment #8559277 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #10) Ok, so this didn't fix the problem and I still get: TEST-UNEXPECTED-FAIL | testGCAllocator | CHECK failed: positionIsCorrect("xxooxxx---------", stagingArea, chunkPool, tempChunks)
Comment 12•9 years ago
|
||
Hmm, that's odd. Could the background task somehow *start* after rt->gc.nursery.waitBackgroundFreeEnd()? The allocator test basically needs the 'staging area' to be contiguous, and for there to be no usable chunksized regions before it (fillSpaceBeforeStagingArea()). If the GC allocates into the staging area while the test is running, or if holes open up before it, the test will fail.
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #12) No, I added logging and the background free runs once before the test starts but not during it. The chunk allocated is some distance after the staging area. I tried adding a loop to retry the allocation if it was not the expected one, and then a chunk was allocated in the staging area, but not in the expected place.
Comment 14•9 years ago
|
||
Actually, I think it's misdetecting the address growth direction: the test failure is on Linux, and I believe mmap addresses usually grow down (not necessarily on every distro, and *not* on OSX), which doesn't match the failure you're getting (which is from the testGCAllocatorUp() path). The test it does to figure out which direction to use (addressesGrowUp()) isn't very robust: it simply allocates two chunks and compares their relative addresses. The actual allocator does something better, but it depends on getting a misaligned chunk in the first place. Even so, I'm surprised this is failing now, especially after your fix to wait for the background free. I'm also not sure how to make it better except by allocating a bunch of chunks and seeing which direction is more likely.
Comment 15•9 years ago
|
||
The only thing I can think of is that munmap *also* works asynchronously, and as a result waiting for the GC's activity to finish isn't quite enough. In that case just sleeping for a few ms will probably work, annoying as that would be. As I said, the actual allocator shouldn't be affected - it's just the test being sensitive to address space changes. That is, assuming my analysis is correct.
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #15) Thanks for the suggestion, I'll try adding a sleep and see if that fixes it.
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #16) No, it still fails :(
Comment 18•9 years ago
|
||
I tried reproducing this locally in a 64-bit and then a 32-bit VM of Linux Mint, but no luck. It would be nice to confirm my hypothesis on the wrong direction being chosen - you could hardcode it to use testGCAllocatorDown() - but it looks like I'll have to use tryserver to do any debugging on this myself.
Assignee | ||
Comment 19•9 years ago
|
||
Here's a patch to make this test more robust in the way it determines which direction memory is allocated in.
Attachment #8559277 -
Attachment is obsolete: true
Attachment #8571974 -
Flags: review?(terrence)
Comment 20•9 years ago
|
||
Comment on attachment 8571974 [details] [diff] [review] bug1122640-testGCAllocator Review of attachment 8571974 [details] [diff] [review]: ----------------------------------------------------------------- Great!
Attachment #8571974 -
Flags: review?(terrence) → review+
Comment 21•9 years ago
|
||
Thanks! I was considering something like this, but I gave up after failing to reproduce the failures myself.
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c2473a9d7ba
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•