Closed Bug 1384049 Opened 2 years ago Closed 2 years ago

Malloc bytes threshold triggers a non-incremental GC

Categories

(Core :: JavaScript: GC, defect, P3)

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [qf:p2])

Attachments

(1 file)

For the GC bytes trigger we start an incremental collection when a zone reaches some fraction of the trigger threshold and a non-incremental collection if we reach the full value.  We currently don't do this for the malloc bytes trigger and only trigger non-incremental collections.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p2]
Priority: -- → P3
Assignee: pbone → jcoppeard
Depends on: 1405274
Patch to add an incremental GC triggers for the malloc bytes counters.

Firstly this renames a few things, removing the word 'zone' from GCRuntim::maybeAllocTriggerZoneGC() and the zoneAllocThresholdFactor parameters now they can trigger/relate to full GCs as well as zone GCs.

Then this adds triggers for both zone GC and full GC based on the zone / runtime malloc counters.  We use the same factor as for the zone GCs so we trigger an incremental GC at 90% of the threshold.

I ran some benchmarks and this didn't seem to affect anything.  I checked in a browser build that this does in fact trigger incremental GCs.
Attachment #8914787 - Flags: review?(pbone)
Comment on attachment 8914787 [details] [diff] [review]
bug1384049-incremental-malloc-trigger

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

I'm curious about the talos/awfy results, however incremental GC is generally a good thing so I'm happy to see those after the patch lands.

The patch itself is good and can land now. You can optionally address my request for a comment below.

Thanks.

::: js/src/jsgc.cpp
@@ +3223,5 @@
> +        return;
> +    }
> +
> +    mallocBytes = zone->GCMallocBytes();
> +    mallocThesholdBytes = zone->GCMaxMallocBytes() * zoneAllocThresholdFactor;

Is there a reason why the zone trigger uses zoneAllocThresholdFactor while the whole-heap threshold uses tunables.allocThresholdFactor?  I have no problem with it but I'd like to know if there's a specific reason / what you were thinking.  If it's interesting could you add a comment?
Attachment #8914787 - Flags: review?(pbone) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/faf7b551ffd6
Trigger an incremental GC based on malloc memory counters r=pbone
The patch seems to have regressed some Octane tests on AWFY
https://hg.mozilla.org/mozilla-central/rev/faf7b551ffd6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Guilherme Lima from comment #4)

Looks like a regression on Box2D and MandreelLatency and an improvement on GameBoy.
Depends on: 1406023
Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/afb1ab8386020882b1920b8015704f4e13ac4245
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla58 → ---
Sorry, got my bugzilla tabs mixed up. This is the merged backout: https://hg.mozilla.org/mozilla-central/rev/afb1ab838602
Keywords: perf
when this was backed out we saw a regression in AWSY data:
== Change summary for alert #9892 (as of October 07 2017 11:12 UTC) ==

Regressions:

  3%  Heap Unclassified summary osx-10-10 opt      66,719,710.96 -> 68,551,454.80

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9892

looking forward to this landing again to get the improvement
Following the change in bug 1406065 to make the shell max malloc parameter the same as for the browser, I get these octane results locally:

         Richards:       30924.0 (10  1.1%)       31067.8 (10  0.9%)   0.5%
        DeltaBlue:       69123.0 (10  0.6%)       68846.9 (10  0.7%)  -0.4%
           Crypto:       30097.6 (10  0.5%)       29854.8 (10  1.5%)  -0.8%
         RayTrace:      131858.6 (10  5.6%)      129202.1 (10 11.4%)  -2.0%
      EarleyBoyer:       37513.3 (10  2.7%)       37315.2 (10  2.5%)  -0.5%
           RegExp:        4998.6 (10  0.7%)        4971.2 (10  1.4%)  -0.5%
            Splay:       14986.5 (10 10.0%)       15993.2 (10  3.4%)   6.7%
     SplayLatency:       19639.9 (10 14.8%)       21361.8 (10  2.6%)   8.8%
     NavierStokes:       43029.4 (10  2.6%)       43552.2 (10  0.2%)   1.2%
            PdfJS:       19959.8 (10  3.9%)       20322.8 (10  1.9%)   1.8%
         Mandreel:       30386.2 (10  1.7%)       30017.1 (10  3.2%)  -1.2%
  MandreelLatency:       35266.8 (10  1.8%)       35039.9 (10  3.1%)  -0.6%
          Gameboy:       72932.0 (10  3.6%)       74854.1 (10  3.1%)   2.6%
         CodeLoad:       22837.8 (10  2.2%)       22596.9 (10  3.2%)  -1.1%
            Box2D:       54745.0 (10  1.9%)       50482.6 (10 10.3%)  -7.8%
             zlib:       80947.5 (10  6.6%)       80609.7 (10  2.1%)  -0.4%
       Typescript:       25695.4 (10  5.0%)       30255.9 (10  6.2%)  17.7%
Score (version 9):       33398.1 (10  2.0%)       33832.7 (10  1.0%)   1.3%

I'm going to try re-landing this.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c112aed854
Trigger an incremental GC based on malloc memory counters r=pbone
https://hg.mozilla.org/mozilla-central/rev/e0c112aed854
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1408375
The reland spotted these performance improvements:

== Change summary for alert #10032 (as of October 04 2017 15:25 UTC) ==

Improvements:

  2%  Heap Unclassified summary osx-10-10 opt      68,068,299.70 -> 66,625,237.79
  2%  Heap Unclassified summary windows10-64 pgo   46,368,998.78 -> 45,435,734.52
  2%  Heap Unclassified summary windows10-64 opt   46,589,214.68 -> 45,808,793.39

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10032
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #14)
> The reland spotted these performance improvements:

Pardon me. These old improvements were noticed for the first push, mentioned in comment 3.
So *before* the backout which happened in comment 13.
You need to log in before you can comment on or make changes to this bug.