Closed Bug 1122640 Opened 7 years ago Closed 7 years ago

Investigate freeing nursery huge slots off main thread

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

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)

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.
+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
Add a GCParallelTask to free the huge slots off main thread.
Assignee: nobody → jcoppeard
Attachment #8558456 - Flags: review?(terrence)
...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 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+
Also, please make onOutOfMallocMemory join on this task before returning.
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
Fixed busage due to missing explicit on single argument constructor:

https://hg.mozilla.org/integration/mozilla-inbound/rev/52a98fafd551
And backed out for testGCAllocator jsapi test failures on linux 32 bit.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0203370cd4db
Attached patch finish-gc-before-alloc-test (obsolete) — Splinter Review
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 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+
(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)
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.
(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.
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.
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.
(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.
(In reply to Jon Coppeard (:jonco) from comment #16)
No, it still fails :(
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.
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 on attachment 8571974 [details] [diff] [review]
bug1122640-testGCAllocator

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

Great!
Attachment #8571974 - Flags: review?(terrence) → review+
Thanks! I was considering something like this, but I gave up after failing to reproduce the failures myself.
https://hg.mozilla.org/mozilla-central/rev/4c2473a9d7ba
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1140773
You need to log in before you can comment on or make changes to this bug.