1244279 - Investigate stealing a bit on ObjectElements to signify whether the object is in the whole cell buffer

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

If the object is already in the whole cell buffer, then adding SlotsEdges for that object is just adding unnecessary work.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

No time to look into this at the moment, but it is still definitely something to investigate when someone gets some cycles.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Found some cycles in between other things.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Add a GC ubench for large arrays with both elements and properties

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

Haven't ran the numbers yet, so I don't know if this is a win or not yet. Stay
tuned for more...

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: ubench with patch

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: ubench without patch

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looks like a win to me!

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8746181 [details] [diff] [review]
Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

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

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

Many header include order! Wow! Such -inl.h!

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=852228d11305

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8746225 [details] [diff] [review]
Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

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

::: js/src/jit/IonCaches.cpp
@@ +5041,5 @@
>      // Monitor changes to cache entry.
>      TypeScript::Monitor(cx, script, pc, vp);
>  
>      return true;
>  }

Accidental?

::: js/src/jit/Linker-inl.h
@@ +57,5 @@
> +    masm.link(code);
> +    if (masm.embedsNurseryPointers())
> +        cx->runtime()->gc.storeBuffer.putWholeCell(code);
> +    return code;
> +}

I cannot image that this much code actually needs to be inlined for performance. I expect it was only in a header to avoid the need to manually instantiate. Please just use a .cpp for this instead of an inl.h.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

Moved the template out to Linker.cpp and explicitly instantiated it with CanGC
and NoGC.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb98ac7de73d
https://hg.mozilla.org/integration/mozilla-inbound/rev/796d263c5c19

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/cb98ac7de73d
https://hg.mozilla.org/mozilla-central/rev/796d263c5c19