1396499 - Consider reusing Array object for expression stack values in GeneratorObject::suspend

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

currently, array object is created every time generator suspends,
and discarded every time generator resumes.

it might be nice to cache the array object for subsequent suspend on resume,
with elements cleared.

Comment 1

[Tooru Fujisawa [:arai]]

https://dxr.mozilla.org/mozilla-central/rev/37824bf5c5b08afa7e689fceb935b8f457ebd9eb/js/src/vm/GeneratorObject.cpp#84
> bool
> GeneratorObject::suspend(JSContext* cx, HandleObject obj, AbstractFramePtr frame, jsbytecode* pc,
>                          Value* vp, unsigned nvalues)
> {
> ...
>     ArrayObject* stack = nullptr;
>     if (nvalues > 0) {
>         stack = NewDenseCopiedArray(cx, nvalues, vp);
>         if (!stack)
>             return false;
>     }
> 
> ...
>     if (stack)
>         genObj->setExpressionStack(*stack);

https://dxr.mozilla.org/mozilla-central/rev/37824bf5c5b08afa7e689fceb935b8f457ebd9eb/js/src/vm/GeneratorObject.cpp#178
> bool
> GeneratorObject::resume(JSContext* cx, InterpreterActivation& activation,
>                         HandleObject obj, HandleValue arg, GeneratorObject::ResumeKind resumeKind)
> {
> ...
>     if (genObj->hasExpressionStack()) {
>         uint32_t len = genObj->expressionStack().length();
>         MOZ_ASSERT(activation.regs().spForStackDepth(len));
>         const Value* src = genObj->expressionStack().getDenseElements();
>         mozilla::PodCopy(activation.regs().sp, src, len);
>         activation.regs().sp += len;
>         genObj->clearExpressionStack();

Comment 2

[Tooru Fujisawa [:arai]]

inlined version of clearExpressionStack in baseline.

https://dxr.mozilla.org/mozilla-central/rev/5eba13f5b3a6ad80decdd8c7b30bff5fa477844f/js/src/jit/BaselineCompiler.cpp#4855-4856
>         masm.guardedCallPreBarrier(exprStackSlot, MIRType::Value);
>         masm.storeValue(NullValue(), exprStackSlot);

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: WIP: Reuse Array object for expression stack values in GeneratorObject::suspend.

result of testcase in bug 1393712, with --no-async-stacks

before: 3013 [ms]
after:  2849 [ms] (-5.4%)

I'll ask review after try run.

Comment 4

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=188c379be7c1bd0100253aa0b9b6d8e32261521d

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8918641 [details] [diff] [review]
WIP: Reuse Array object for expression stack values in GeneratorObject::suspend.

apparently the array handling is broken :P

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: WIP: Reuse Array object for expression stack values in GeneratorObject::suspend.

fixed capacity

https://treeherder.mozilla.org/#/jobs?repo=try&revision=639a3ef487d6bf9fb549d7cccd42824238464a21

Comment 7

[Tooru Fujisawa [:arai]]

the patch has bug around pre-barrier on copyDenseElements.
I need to properly cleanup the previous elements and set new elements cleanly, on separate timing.

Comment 8

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5090497dc3708363cd6eee152c19652901cbd8c7

Comment 9

[Tooru Fujisawa [:arai]]

looks like setOrExtendDenseElements does what I want
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39061667d01218f7f35e42b5c32db73cd766aa60

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Reuse Array object for expression stack values in GeneratorObject::suspend.

Changed the following:
  * Cache ArrayObject in expression stack slot
    * Set the ArrayObject's length property to 0, instead of null it out, when resuming
  * Use cached ArrayObject when suspending
    * when successfully set/extend the array object with current expression stack, use the cached array
    * if fail, or when there's no cached array, create new array

performance improvement in the testcase for bug 1393712
  before: ~2800 [ms]
  after:  ~2600 [ms]

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8919286 [details] [diff] [review]
Reuse Array object for expression stack values in GeneratorObject::suspend.

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

Nice, thanks!

::: js/src/jit/BaselineCompiler.cpp
@@ +4836,5 @@
>      masm.branchTestNull(Assembler::Equal, exprStackSlot, &noExprStack);
>      {
>          masm.unboxObject(exprStackSlot, scratch2);
>  
>          Register initLength = regs.takeAny();

Nit: please rename initLength to length or arrayLength as it no longer stores the initialized length.

::: js/src/vm/GeneratorObject.cpp
@@ +186,5 @@
>          MOZ_ASSERT(activation.regs().spForStackDepth(len));
>          const Value* src = genObj->expressionStack().getDenseElements();
>          mozilla::PodCopy(activation.regs().sp, src, len);
>          activation.regs().sp += len;
> +        genObj->expressionStack().setLengthInt32(0);

Note that this will keep the values (the array elements) alive while the generator runs. It's probably fine.

Comment 12

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #11)
> ::: js/src/vm/GeneratorObject.cpp
> @@ +186,5 @@
> >          MOZ_ASSERT(activation.regs().spForStackDepth(len));
> >          const Value* src = genObj->expressionStack().getDenseElements();
> >          mozilla::PodCopy(activation.regs().sp, src, len);
> >          activation.regs().sp += len;
> > +        genObj->expressionStack().setLengthInt32(0);
> 
> Note that this will keep the values (the array elements) alive while the
> generator runs. It's probably fine.

Does it mean that setting length to 0 doesn't release the reference?
should I do something else to release elements?

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #12)
> Does it mean that setting length to 0 doesn't release the reference?
> should I do something else to release elements?

I think you could use setDenseInitializedLength(0) or loop over the elements and set them to |undefined|.

Comment 14

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to Tooru Fujisawa [:arai] from comment #12)
> > Does it mean that setting length to 0 doesn't release the reference?
> > should I do something else to release elements?
> 
> I think you could use setDenseInitializedLength(0) or loop over the elements
> and set them to |undefined|.

thanks.
it sounds like I need to handle barriers inside baseline JIT code.
I'll try to implement that.  if it's not possible with a few change, I'll leave it to other bug.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Reuse Array object for expression stack values in GeneratorObject::suspend. r=jandem

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Do not hold reference to expression stack values in GeneratorObject::suspend.

reverted to use initialized length, and added pre barrier to baseline code.
it's setting length to 0 because setDenseInitializedLength does pre-barrier to [length,initializedLength) range.
(am I correct?)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=361411d0de3562e40fed7049a9b510f5b51fbaba

I see no notable performance difference.

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8920436 [details] [diff] [review]
Part 2: Do not hold reference to expression stack values in GeneratorObject::suspend.

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

Looks good, but:

(In reply to Tooru Fujisawa [:arai] from comment #16)
> it's setting length to 0 because setDenseInitializedLength does pre-barrier
> to [length,initializedLength) range.

Is setting |length| necessary? Note that |length| in setDenseInitializedLength refers to the |uint32_t length| argument, not to the |length| field.

Comment 18

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #17)
> Is setting |length| necessary? Note that |length| in
> setDenseInitializedLength refers to the |uint32_t length| argument, not to
> the |length| field.

wow, thanks for pointing out that.
I'll check I can skip setting it, and remove if unnecessary.

Comment 19

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/555b9d18b9454907455b0e3ec38f9a3e6b4d001a
Bug 1396499 - Part 1: Reuse Array object for expression stack values in GeneratorObject::suspend. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2d7d03c2a25f4af5c8bf81893b197bc238b853
Bug 1396499 - Part 2: Do not hold reference to expression stack values in GeneratorObject::suspend. r=jandem

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/555b9d18b945
https://hg.mozilla.org/mozilla-central/rev/5d2d7d03c2a2