1479360 - Optimize and simplify nursery allocation

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Paul Bone [:pbone]]

Share the JIT bump pointer allocation code between nursery strings allocation and nursery object allocation.  This also optimises nursery object allocation since it now includes fewer literal 64-bit numbers.

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1479360 - Share and optimize bump-pointer allocation code

I noticed that while some effort was made to avoid abosolute addresses in the nursery strings allocation path.  The same wasn't done for other nursery objects, and there were a few absolute addresses here.  This patch uses the same code for both allocations and with some initial testing improves octane by 1.5%.  I'm waiting for the talos jobs now.

Comment 2

[Paul Bone [:pbone]]

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

Perf herder: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=1dd1db8793e5ca89bf5f5e366c52eba00eeef3b8

Comment 3

[Lars T Hansen [:lth]]

Will this code conceivably be used by wasm when wasm has object support?  There must be strictly zero absolute addresses in wasm jit code that can be cached to disk; not saying you can't do this, just that we need to be aware of that.

Comment 4

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

Comment on attachment 8995929 [details] [diff] [review]
Bug 1479360 - Share and optimize bump-pointer allocation code

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

Really r=me, but I guess I'd like to see the final version first, to decide whether I need to get the review double-checked by sumbuddy smarte. Waldo, maybe.

(I'll be working tonight for several hours before our meeting, so we should have lots of overlap and you won't need to wait another day for a review cycle from me.)

::: js/src/jit/MacroAssembler.cpp
@@ +979,3 @@
>      // The nursery position (allocation pointer) and the nursery end are stored
>      // very close to each other -- specifically, easily within a 32 bit offset.
>      // Use relative offsets between them, to avoid 64-bit immediate loads.

This should either add "nursery" to the function name, or (much better) remove it from the comment.

@@ +983,4 @@
>      loadPtr(Address(temp, 0), result);
>      addPtr(Imm32(totalSize), result);
>      const ptrdiff_t endOffset =
> +        uintptr_t(curEndAddr) - uintptr_t(posAddr);

Hm... this is from my pre-existing code, but it seems like we should be asserting away the implementation-defined behavior here.

Say posAddr was a little bit bigger than curEndAddr. Then uintptr_t(curEndAddr) - uintptr_t(posAddr)  is defined behavior and would return a very large unsigned value, but casting it to ptrdiff_t is implemented-defined behavior and might clamp or do something else stupid (in theory).

But I'm not language lawyer. I attempted to come up with a full set of assertions to permit only the desired behavior, and it was taking too long so I decided perhaps it would be best to give up and rely on CheckedInt.

How about

  CheckedInt<uintptr_t> curEndPtr = curEndAddr;
  CheckedInt<uintptr_t> posPtr = posAddr;
  CheckedInt<int32_t> endOffset = curEndAddr - posAddr;
  MOZ_ASSERT(endOffset.isValid());
  branchPtr(Assembler::Below, Address(temp, endOffset.value()), result, fail);

It would be nice to have something more static.

Note that my original optimization eliminated one more 64-bit constant in most cases, by checking whether the Zone pointer was also close enough -- and I got it completely wrong, such that the optimization was statically provable to never be applied. :( So don't trust me on any of this!

::: js/src/jit/MacroAssembler.h
@@ +2280,5 @@
>      void nurseryAllocateObject(Register result, Register temp, gc::AllocKind allocKind,
>                                 size_t nDynamicSlots, Label* fail);
> +    void bumpPointerAllocate(Register result, Register temp, Label* fail,
> +        void* posAddr, const void* curEddAddr,
> +        uint32_t totalSize, uint32_t size);

It seems like the standard parameter order in this file puts the fail label last.

Comment 5

[Paul Bone [:pbone]]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)
> Comment on attachment 8995929 [details] [diff] [review]
> Bug 1479360 - Share and optimize bump-pointer allocation code
> 
> Review of attachment 8995929 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Really r=me, but I guess I'd like to see the final version first, to decide
> whether I need to get the review double-checked by sumbuddy smarte. Waldo,
> maybe.
> 
> (I'll be working tonight for several hours before our meeting, so we should
> have lots of overlap and you won't need to wait another day for a review
> cycle from me.)

Okay cool.  Yeah the 'overnight reviews' is sometimes tricky.  Sometimes I work late to prepare a patch that evening so I know I don't have to wait another day.  If I can manage it a good solution will be to keep several patches to several bugs in-flight.

> ::: js/src/jit/MacroAssembler.cpp
> @@ +983,4 @@
> >      loadPtr(Address(temp, 0), result);
> >      addPtr(Imm32(totalSize), result);
> >      const ptrdiff_t endOffset =
> > +        uintptr_t(curEndAddr) - uintptr_t(posAddr);
> 
> Hm... this is from my pre-existing code, but it seems like we should be
> asserting away the implementation-defined behavior here.
> 
> Say posAddr was a little bit bigger than curEndAddr. Then
> uintptr_t(curEndAddr) - uintptr_t(posAddr)  is defined behavior and would
> return a very large unsigned value, but casting it to ptrdiff_t is
> implemented-defined behavior and might clamp or do something else stupid (in
> theory).

I changed the types here from your original code, and I have other patches that add assertions in my workspace.  AIUI ptrdiff_t is always the same size as uintptr_t, the only difference is that ptrdiff_t is signed.  I can still see problems if one address is near 0 and the other is near 1<<63.  So CheckedInt might make everyone feel safer.

> But I'm not language lawyer. I attempted to come up with a full set of
> assertions to permit only the desired behavior, and it was taking too long
> so I decided perhaps it would be best to give up and rely on CheckedInt.
> 
> How about
> 
>   CheckedInt<uintptr_t> curEndPtr = curEndAddr;
>   CheckedInt<uintptr_t> posPtr = posAddr;
>   CheckedInt<int32_t> endOffset = curEndAddr - posAddr;
>   MOZ_ASSERT(endOffset.isValid());
>   branchPtr(Assembler::Below, Address(temp, endOffset.value()), result,
> fail);
> 
> It would be nice to have something more static.

Maybe, but it might need to be added to the Nursery class, and rather than return pointers to both the current end and position, instead return one as an offset from the other, and do the static check in the Nursery class.

> Note that my original optimization eliminated one more 64-bit constant in
> most cases, by checking whether the Zone pointer was also close enough --
> and I got it completely wrong, such that the optimization was statically
> provable to never be applied. :( So don't trust me on any of this!

Yes, I saw that, but I didn't follow through enough to convice myself that it was broken.  We may be able to address that, and maybe even Lars' query if we re-arrange these structures and place all three fields into one structure.  While thinking of that, Lars, is there one absolute address pointing to a structure with everything wasm will already have?  or maybe a register for this purpose?  Something like a thread local data register?

Comment 6

[Paul Bone [:pbone]]

I've done some better tests.  I get a 2.1% improvment on octane in the shell.  with 8 samples before and after the change.  It is statistically sagnificant with p = 0.0394

Comment 7

[Paul Bone [:pbone]]

Attachment: Bug 1479360 - Share and optimize bump-pointer allocation code

The assertions are fixed in Bug 1478902 and apply before this patch within my workspace.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a914cedebde5
Share and optimize bump-pointer allocation code r=sfink

Comment 9

[Paul Bone [:pbone]]

Overholt asked if I could test this with speedometer.  It makes a 2.4% improvment to speedometer when I test locally.  p=0.0004  :D :D

Comment 10

[Dorel Luca [:dluca]]

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

Comment 11

[Andrew Overholt [:overholt]]

(In reply to Paul Bone [:pbone] from comment #9)
> Overholt asked if I could test this with speedometer.  It makes a 2.4%
> improvment to speedometer when I test locally.  p=0.0004  :D :D

\o/

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #9)
Nice!  

Also this shows that we do need to be careful when adding anything to the allocation fast path.

Comment 13

[Paul Bone [:pbone]]

:jonco  True, I'm already going to make an improvment to Bug 1473213 based on what I learnt here.