1380778 - Speedometer triggers minorGC very often

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

https://perfht.ml/2vhxjmh

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: minor gc profiling output

This is the log of the minor GCs. The histogram of reasons is:

      1 DOM_WORKER
      1 LOAD_END
      2 API
      3 EAGER_ALLOC_TRIGGER
      6 INTER_SLICE_GC
     11 ALLOC_TRIGGER
     16 OUT_OF_NURSERY
     20 FULL_STORE_BUFFER

Total time (in microseconds) grouped by reason:

362 DOM_WORKER
3047 API
3657 LOAD_END
6655 EAGER_ALLOC_TRIGGER
30285 ALLOC_TRIGGER
33590 INTER_SLICE_GC
68648 OUT_OF_NURSERY
81462 FULL_STORE_BUFFER

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)
>      20 FULL_STORE_BUFFER

It would be interesting to know which store buffer this is, and why we end up with a lot of entries in it..

Comment 3

[Jon Coppeard (:jonco)]

I made a patch to add separate reasons for each store buffer and got the following results:

 359 FULL_GENERIC_BUFFER
 101 FULL_CELL_PTR_BUFFER
  69 FULL_WHOLE_CELL_BUFFER
  52 FULL_SLOT_BUFFER

So it wasn't the cell pointer buffer as I had guessed but the generic buffer that's filling up.

Comment 4

[Jon Coppeard (:jonco)]

We use the generic buffer for updating shape accessor getter/setter objects, Map/Set objects containing nursery pointers and ConstraintTypeSets.  Looking at counts for generic buffer entries I get:

69553 GenericRef ShapeGetterSetterRef
 2871 GenericRef TypeSetRef
  542 GenericRef OrderedHashTableRef

In this case, it looks like accessor shapes are the culprit.

Comment 5

[Jon Coppeard (:jonco)]

I experimented with changing shape hashing to use unique IDs and then using the whole cell store buffer to update a shape's nursery pointers.  This hit problems with sweep order: the unique IDs table is swept before we sweep arenas containing shapes, so it's not possible for a shape's parent to look up a dead child to remove it if we do this.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1380778-accessor-shape-vector

Instead, here's a patch to keep a vector of accessor shapes containing nursery pointers on the zone and trace them in one go on minor GC.  This avoids adding a generic ref for each shape as we did previously.

Minor GC counts running speedometer with this patch:

  85 FULL_CELL_PTR_BUFFER
  79 FULL_SLOT_BUFFER
  51 FULL_WHOLE_CELL_BUFFER
  17 FULL_GENERIC_BUFFER

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6bf5093ad73fc9665ea17b1bec0ec5c2a11ce0b

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8886656 [details] [diff] [review]
bug1380778-accessor-shape-vector

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

Wow. That seems to have totally nailed it.

::: js/src/vm/Shape-inl.h
@@ +157,5 @@
> +
> +    auto& storeBuffer = shape->runtimeFromActiveCooperatingThread()->gc.storeBuffer();
> +    if (nurseryShapes.length() == 1) {
> +        storeBuffer.putGeneric(NurseryShapesRef(shape->zone()));
> +    } if (nurseryShapes.length() == MaxShapeVectorLength) {

missing 'else'

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c73b22611c
Fixup shape pointers into the nursery using a dedicated list r=sfink

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe95017106a
Fix review comment I missed r=sfink

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/d0c73b22611c
https://hg.mozilla.org/mozilla-central/rev/fbe95017106a