Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Description • 7 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 1 • 7 years ago

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.

	Paul Bone [:pbone]
	Comment 2 • 7 years ago

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?

	Pulsebot
	Comment 3 • 7 years ago

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

	Guilherme Lima
	Comment 4 • 7 years ago

The patch seems to have regressed some Octane tests on AWFY

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 5 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/faf7b551ffd6

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 6 • 7 years ago

(In reply to Guilherme Lima from comment #4)

Looks like a regression on Box2D and MandreelLatency and an improvement on GameBoy.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 7 • 7 years ago

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/afb1ab8386020882b1920b8015704f4e13ac4245

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 8 • 7 years ago

Merged backout https://hg.mozilla.org/mozilla-central/rev/2d7b8b5dd174

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 9 • 7 years ago

Sorry, got my bugzilla tabs mixed up. This is the merged backout: https://hg.mozilla.org/mozilla-central/rev/afb1ab838602

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 10 • 7 years ago

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

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 11 • 7 years ago

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.

	Pulsebot
	Comment 12 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e0c112aed854

	Ionuț Goldan [:igoldan]
	Comment 14 • 7 years ago

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

	Ionuț Goldan [:igoldan]
	Comment 15 • 7 years ago

(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.