Closed Bug 1032750 Opened 6 years ago Closed 5 years ago

Triggers are updated for non-collected zones on GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(wmccloskey)
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.
Attachment #8448624 - Flags: review+
Flags: needinfo?(wmccloskey)
In the browser, we occasionally do a full GC on a timer in nsJSEnvironment.cpp, so we shouldn't leak forever.
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
It seems it's this patch that causes the test failures, rather than the others that were pushed with it.
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.
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.
Attachment #8448624 - Attachment is obsolete: true
Attachment #8542193 - Flags: review?(terrence)
This will be nice to get landed now that we're actually doing non-full GCs a reasonable amount of the time. ;)
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.
Attachment #8542193 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/87fe5bc1233f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1116376
You need to log in before you can comment on or make changes to this bug.