Closed Bug 1130475 Opened 7 years ago Closed 7 years ago

Move IGC LastDitch GC up to CheckAllocatorState

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

I've also added a new reason here, since it seems odd that LAST_DITCH meant both, "we actually OOMed" and "we hit our ALLOC_TRIGGER while in the middle of IGC".
Attachment #8560565 - Flags: review?(sphink)
Attached patch 2.2_cleanup_refill-v0.diff (obsolete) — Splinter Review
And with that we can finally kill off the last loop and its ridiculous extra bool state vars from refillFreeListFromMainThread \o/.
Attachment #8560567 - Flags: review?(sphink)
(In reply to Terrence Cole [:terrence] from comment #0)
> Created attachment 8560565 [details] [diff] [review]
> 1.2_lift_igc_last_ditch-v0.diff
> 
> I've also added a new reason here, since it seems odd that LAST_DITCH meant
> both, "we actually OOMed" and "we hit our ALLOC_TRIGGER while in the middle
> of IGC".

Actually, INCREMENTAL_ALLOC_TRIGGER is a /terrible/ name to give to this as it's explicitly NONINCREMENTAL. Uhg. Suggestions welcome.
Target Milestone: flash10 → ---
Comment on attachment 8560565 [details] [diff] [review]
1.2_lift_igc_last_ditch-v0.diff

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

This change makes sense and is *so* much nicer than what it was.
Attachment #8560565 - Flags: review?(sphink) → review+
Comment on attachment 8560567 [details] [diff] [review]
2.2_cleanup_refill-v0.diff

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

::: js/src/jsgc.cpp
@@ +2975,5 @@
> +    if (!allowGC)
> +        return nullptr;
> +
> +    if (void *thing = RunLastDitchGC(cx, zone, thingKind))
> +        return thing;

This one I'm unsure of.

Trivially, this is a shadowing declaration of 'thing'. You could just remove the declaration, or if you prefer it could be

  if (!allowGC || (thing = RunLastDitchGC(...))
    return thing;

but I'm not sure how well that reads. Also, it seems a little odd that RunLastDitchGC both does a GC and tries to allocate the thing afterwards. It seems like it would be clearer to make this

  if (!allowGC)
    return nullptr;

  RunLastDitchGC(cx, zone);

  size_t thingSize = Arena::thingSize(thingKind);
  thing = arenas->allocateFromFreeList(thingKind, thingSize);
  if (thing)
    return thing;

at which point I'm not sure if having a separate RunLastDitchGC function is necessary.

But that's all cosmetic refactoring. The one thing that worries me is that it looks like this refactoring could OOM when the original wouldn't. Specifically if the last ditch GC starts up a background sweeping task, then the new code won't wait for it. Unless LAST_DITCH somehow implies synchronous GC somehow? I don't see it.

Maybe the thing to do is to make RunLastDitchGC always wait. Except that would be introducing a new wait in the case that it does free enough on the foreground already, so you'd want it to try to alloc internally after all, in which case maybe all of this boils down to renaming it to LastDitchAlloc (or LastDitchGCAndAlloc) and conditionally waiting for the background sweep if it's going to return nullptr.

Either that, or I'm just missing something.
Attachment #8560567 - Flags: review?(sphink)
Nope, I missed that we actually expect to go around the loop again. :-/

This patch features a more radical refactoring: abstract the alloc;wait;retry logic into tryRefillFreeList then call that as try;gc;retry. This gets us our 4 alloc calls with a GC in the middle without either the artificial loop or any copy-and-paste.

I've also "enhanced" the LAST_DITCH GC: it now does a GC_SHRINK (since that is now /very/ likely to help) and it does it on all zones, rather than just the allocation zone. I totally do not understand why our "oh crap we're out of memory" GC did not try to reclaim bytes from the rest of the system? I also don't understand why we AutoKeepAtoms? I'd like to remove that too, but I don't want to make too big a change at once. I'll file a followup bug and ni? billm on that one.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=254ffde43bb4
Attachment #8560567 - Attachment is obsolete: true
Attachment #8561510 - Flags: review?(sphink)
And update the comment like I should have in part 1 of this bug. There's also a tiny bugfix to use the already-available cx in the allocator, which I figured I could fold in here.
Attachment #8561512 - Flags: review?(sphink)
Comment on attachment 8561510 [details] [diff] [review]
2.2_cleanup_refill-v1.diff

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

Ok, that's a lot easier to follow and verify.

The only thing that makes me at all nervous is the switch to GC_SHRINK (which I forgot to mention; that seemed odd that we weren't already doing that). It's a big enough behavior change that we might want it in a separate push.
Attachment #8561510 - Flags: review?(sphink) → review+
Comment on attachment 8561512 [details] [diff] [review]
2.3_fixup_comment-v0.diff

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

Very nice comment, though I hadn't seen the patch where you introduced that enum. I guess that's the new name you picked for INCREMENTAL_ALLOC_TRIGGER. Sounds good to me.
Attachment #8561512 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink, :s:] from comment #8)
> Comment on attachment 8561510 [details] [diff] [review]
> 2.2_cleanup_refill-v1.diff
> 
> Review of attachment 8561510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, that's a lot easier to follow and verify.
> 
> The only thing that makes me at all nervous is the switch to GC_SHRINK
> (which I forgot to mention; that seemed odd that we weren't already doing
> that). It's a big enough behavior change that we might want it in a separate
> push.

Good point! I'll go ahead and separate that.
So, it seems we crash in this new code:

bp-4c1c0fd4-c65d-4072-b893-5cbaf2150217
And this signature looks very similar to another crash signature I've previously seen in bug 1114079.
Depends on: 1146696
You need to log in before you can comment on or make changes to this bug.