1032750 - Triggers are updated for non-collected zones on GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: dont-update-triggers-for-uncollected-zones

After a GC, we update the allocation triggers for all zones rather than just the ones we collected.

It seems like this could lead to a situation where a zone that sees frequently allocation would be collected often, and that other more slowly allocated zones might not be collected at all.

Bill, do you know if this is intended behaviour?

The attached patch changes this to only update triggers for collected zones.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8448624 [details] [diff] [review]
dont-update-triggers-for-uncollected-zones

Yeah, this seems bad. I don't know of any reason why it should work this way.

Comment 2

[Andrew McCreight [:mccr8]]

In the browser, we occasionally do a full GC on a timer in nsJSEnvironment.cpp, so we shouldn't leak forever.

Comment 3

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14f0326f6c77

Comment 4

[Carsten Book [:Tomcat]]

sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42936365&tree=Mozilla-Inbound starting with this patch included

Comment 5

[Jon Coppeard (:jonco)]

It seems it's this patch that causes the test failures, rather than the others that were pushed with it.

Comment 6

[Jon Coppeard (:jonco)]

The test failure is js1_5/Regress/regress-360969-05.js timeout on Android 2.3 ARMv6 only.  Interestingly, a GC is running when the test is killed.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: dont-update-triggers-on-uncollected-zones v2

Try build looks ok for this now, so I'm going to try and re-land it.

Re-requesting review because this patch contains some tidyups that weren't in the first version.

Comment 8

[Andrew McCreight [:mccr8]]

This will be nice to get landed now that we're actually doing non-full GCs a reasonable amount of the time. ;)

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 8542193 [details] [diff] [review]
dont-update-triggers-on-uncollected-zones v2

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

::: js/src/jsgc.cpp
@@ +3311,2 @@
>      if (zone->usage.gcBytes() > 1024 * 1024 &&
> +        zone->isCloseToAllocTrigger(schedulingState.inHighFrequencyGCMode()) &&

In my head I had envisioned the flow here to be more like:

if (zone->threshold.hasLargeHeap(zone->usage) &&
    zone->threshold.isCloseToAllocTrigger(zone->usage, schedulingState) &&
    etc...)

But I'm fine landing it like this, since it's definitely better.

@@ +3313,1 @@
>          incrementalState == NO_INCREMENTAL &&

Also, I totally hate our open coding of "is not in a GC" as "incrementalState == NO_INCREMENTAL": it is a real readability issue.

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87fe5bc1233f

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/87fe5bc1233f