1530372 - Allow BigInt values with inline digits to be allocated in the nursery

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P4)

Description

[Andy Wingo [:wingo]]

BigInt values with inline digits should be nursery-allocatable.

BigInt values with inline digit are bigints whose absolute value is less than 2^64 (or 2^96 for 32-bit targets). These values have no finalizer and so are well-suited to nursery allocation.

This would be a nice speed boost for accessing TypedArrays with 64-bit values.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1530372 - Part 4: Support nursery allocation for BigInt. r=sfink!,jandem!

Comment 2

[André Bargull [:anba]]

Added a proof-of-concept patch for BigInt nursery allocation. The patch still has a couple of crude workarounds which should be fixed before landing. And as a follow-up some functionality should be shared across String and BigInt nursery allocation to avoid code duplication (for the most part I've simply copied the existing nursery allocation functions for Strings and then adapted them to work with BigInts).

But first things first: Do we actually want to support nursery allocation for BigInts at this point?

Comment 3

[Andy Wingo [:wingo]]

FWIW to me it doesn't make sense to nursery-allocate bigints with heap digits, because it adds finalization work to the nursery. My instincts (but no benchmarks!) say to only nursery-allocate bigints with inline digits.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Andy Wingo [:wingo] from comment #3)

nursery-allocate bigints with heap digits, because it adds finalization work to the nursery

We could nursery-allocate (instead of malloc) the heap digits in that case. We do this also for object slots/elements.

That said, nursery-allocating BigInts without heap digits would be an excellent starting point and should cover a lot of BigInts :-)

Comment 5

[André Bargull [:anba]]

The current patch allocates the heap digits themselves in the nursery if the BigInt is nursery allocated. This works similar to how object elements and slots are already allocated. That way no extra finalization work has to be added to the nursery.

Comment 6

[Andy Wingo [:wingo]]

(In reply to André Bargull [:anba] from comment #5)

The current patch allocates the heap digits themselves in the nursery if the BigInt is nursery allocated. This works similar to how object elements and slots are already allocated. That way no extra finalization work has to be added to the nursery.

Neat, I didn't see this. Also, forgot to say: nice patch!

Comment 7

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

I'm fine with doing this, but it'll require someone crazy enough to thread it throughout the engine.

Oh, wait, you already did that. :-)

Comment 8

[Jon Coppeard (:jonco)]

Yeah, this is great work. Real data would be preferable but my gut feel is that this is a good thing.

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1530372 - Part 1: Change BigInt to an in-line trace kind. r=sfink!

BigInts need to use 0x01 as their TraceKind value, but that value is
currently already used for Scripts. Therefore move Script to an
out-of-line trace kind to free up 0x01 for BigInts.

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1530372 - Part 2: Prepare Cell flags to support BigInt nursery cells. r=sfink!

Removes the static_assert because it's too restrictive, only nursery cells are
required to reserve Cell::ReservedBits bits in the first word of a cell. For
tenured cells, we only need to use a single bit in the first word. And the
static_assert will also start to fail on 32-bit when Cell::ReservedBits is
changed to 3 to account for nursery BigInts.

Also added an extra assertion to RelocationOverlay::forwardTo so it's easier
to see that storing the gc-flags doesn't overwrite any set bits.

Depends on D56198

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1530372 - Part 3: Assert trace-kind is consistent with forwarded cells. r=sfink!

The trace-kind of forwarded cells must match the trace-kind of their target. Add
an assertion to ensure this invariant isn't violated.

Depends on D56199

Comment 12

[André Bargull [:anba]]

(In reply to Jon Coppeard (:jonco) from comment #8)

Yeah, this is great work. Real data would be preferable but my gut feel is that this is a good thing.

Bug 1599465 improved BigInt operations which fit in uint64_t quite a bit, for example adding 1n + 2n no longer allocates two BigInts and requests malloc memory. Supporting nursery allocation on top of that improves µ-benchmarks for small BigInts by maybe 25%. Nursery allocation for BigInts really shines when we were previously allocating malloc memory for BigInt heap digits, because now we can avoid GC pauses. For example adding (2n << 96n) + 10n is now three to four times faster for me when no GC pauses are present (*), and more than ten times faster when GC pauses are present. (**)

(*) GC pauses when allocating BigInts in the tenured heap. When nursery allocating, the run time is constant and not interrupted by GC pauses.
(**) V8 and JSC are still 2-3 times faster than us, even when nursery allocation is used. So there's still some optimisation work needed. The gap is closer for small BigInts (only 30%-70% slower).

Comment 13

[André Bargull [:anba]]

Try builds look good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1f3e5218584ab741d84859cc4891c0dabcf0872

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1530372 - Part 5: Add 'nursery-bigints' to fuzz-flags.txt. r=gkw!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b2680319bf7
Part 1: Change BigInt to an in-line trace kind. r=sfink
https://hg.mozilla.org/integration/autoland/rev/0366a2f516d4
Part 2: Prepare Cell flags to support BigInt nursery cells. r=sfink
https://hg.mozilla.org/integration/autoland/rev/9ae1e055de13
Part 3: Assert trace-kind is consistent with forwarded cells. r=sfink
https://hg.mozilla.org/integration/autoland/rev/a0d1fb0a86b0
Part 4: Support nursery allocation for BigInt. r=sfink,jandem
https://hg.mozilla.org/integration/autoland/rev/2e7c376532af
Part 5: Add 'nursery-bigints' to fuzz-flags.txt. r=gkw

Comment 16

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/3b2680319bf7
https://hg.mozilla.org/mozilla-central/rev/0366a2f516d4
https://hg.mozilla.org/mozilla-central/rev/9ae1e055de13
https://hg.mozilla.org/mozilla-central/rev/a0d1fb0a86b0
https://hg.mozilla.org/mozilla-central/rev/2e7c376532af