Closed Bug 1366375 Opened 7 years ago Closed 7 years ago

BaselineJIT: Add IC stub for array_push

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Performance Impact high
Tracking Status
firefox56 --- fixed

People

(Reporter: djvj, Assigned: djvj)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 5 obsolete files)

This is one of the top time-used functions.  I know this is inlined in Ion, under common-case fastpath conditions, but baseline doesn't have an IC for it, so it goes unoptimized.

The optimizatable case here is:

if:
target.isObject()
numArgs == 1
targetObj.clasp() is Array
targetObj is dense array

  if:
  targetObj.length < targetObj.capacity

  then:
  targetObj[length] = arg[0]
  targetObj.length++

  else:
  callDenseArrayPushKernel(targetArray, targetObj)

else:
gotoNextICStub()

Should be a straightforward thing to avoid the call to this in common cases.
Whiteboard: [qf] → [qf:p1]
Attached patch optimize-array-push.patch (obsolete) — Splinter Review
Optimizes array.push in baseline.  Might require some cleanup but passes jit-tests and jsreftests on my machine.

There's a bit of boilerplate here for allowing CacheIR "Call" stubs to choose their own stub kind (Regular/Monitored/Updated) instead of having it dictated by the CacheIR stub kind.
Attachment #8884129 - Flags: review?(jdemooij)
Comment on attachment 8884129 [details] [diff] [review]
optimize-array-push.patch

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

Looks reasonable, some suggestions below. The main thing is that I think we shouldn't optimize ArrayPush after the call - I've been trying to avoid that with the CacheIR stubs because it tends to be more complicated than checking before performing the operation.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +2086,5 @@
>  jit::AttachBaselineCacheIRStub(JSContext* cx, const CacheIRWriter& writer,
>                                 CacheKind kind, ICStubEngine engine, JSScript* outerScript,
>                                 ICFallbackStub* stub, bool* attached)
>  {
> +    return AttachBaselineCacheIRStub(cx, writer, kind, /* stubKind = */ ICStub::INVALID,

Iit would be simpler if all callers of AttachBaselineCacheIRStub passed in the CacheIRStubKind. Then we can remove the switch statement below (we still want a switch or if-else to determine stubDataOffset but that will only need 3 cases).

::: js/src/jit/BaselineIC.cpp
@@ +2514,5 @@
>      bool optimizeAfterCall = false;
>      CallIRGenerator::OptStrategy optStrategy = gen.getOptStrategy(&optimizeAfterCall);
>  
> +    // If the optStrategy is ArrayPush, and we're pushing too many, don't try to
> +    // optimize it that way anymore.

The CacheIR way to handle this is to check ICState::Mode and only optimize if it's Mode::Specialized

::: js/src/jit/CacheIR.cpp
@@ +3724,5 @@
> +    RootedArrayObject thisarray(cx_, &thisobj->as<ArrayObject>());
> +    if (!thisarray->nonProxyIsExtensible())
> +        return false;
> +
> +    // The ObjectGroup for the this object may have changed during the call.

How can it change?
Attachment #8884129 - Flags: review?(jdemooij)
Attached patch optimize-array-push.patch (obsolete) — Splinter Review
(In reply to Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 8884129 [details] [diff] [review]
> optimize-array-push.patch
> 
> Review of attachment 8884129 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable, some suggestions below. The main thing is that I think we
> shouldn't optimize ArrayPush after the call - I've been trying to avoid that
> with the CacheIR stubs because it tends to be more complicated than checking
> before performing the operation.
> 
> ::: js/src/jit/BaselineCacheIRCompiler.cpp
> @@ +2086,5 @@
> >  jit::AttachBaselineCacheIRStub(JSContext* cx, const CacheIRWriter& writer,
> >                                 CacheKind kind, ICStubEngine engine, JSScript* outerScript,
> >                                 ICFallbackStub* stub, bool* attached)
> >  {
> > +    return AttachBaselineCacheIRStub(cx, writer, kind, /* stubKind = */ ICStub::INVALID,
> 
> Iit would be simpler if all callers of AttachBaselineCacheIRStub passed in
> the CacheIRStubKind. Then we can remove the switch statement below (we still
> want a switch or if-else to determine stubDataOffset but that will only need
> 3 cases).

The issue with call-stubs needing to be attached before invocation has been problematic to deal with, but it seems useful to explicitly preserve a mechanism for after-call stub attachment for the cases that need it.  We haven't specifically run across it yet, but I can envision wanting to get at information after the call to use in the optimization (shape changes on a particular object, object-group of returned value, etc.).

Both of the current examples technically don't need information after the call (StringSplit now uses a fixed group, and ArrayPush doesn't need any info after call), the overall infrastructure for performing checks before the call, deciding to optimize after the call, and skipping pre-call stub addition, seems useful to preserve.

If you feel strongly that we should get rid of the before/after split for these, I can oblige, but I feel we should keep it as is to preserve that codepath for future expansion.

> The CacheIR way to handle this is to check ICState::Mode and only optimize
> if it's Mode::Specialized

Done.


> ::: js/src/jit/CacheIR.cpp
> @@ +3724,5 @@
> > +    RootedArrayObject thisarray(cx_, &thisobj->as<ArrayObject>());
> > +    if (!thisarray->nonProxyIsExtensible())
> > +        return false;
> > +
> > +    // The ObjectGroup for the this object may have changed during the call.
> 
> How can it change?

I remember there being cases where an Object's ObjectGroup can change.  I figure whenever we re-enter the engine, something like that can happen.

I'm guessing it can't, but can you tell me why?  ArrayObjects never change groups?  (I've removed that line in my latest patch).
Attachment #8884129 - Attachment is obsolete: true
Attachment #8887663 - Flags: review?(jdemooij)
jandem: Never mind about that last question about why group changes can't happen.  Just figured it out myself :)
Attached patch optimize-array-push.patch (obsolete) — Splinter Review
Previous attachment was just the top of a temporary patch queue.  This is the full merged patch.
Attachment #8887663 - Attachment is obsolete: true
Attachment #8887663 - Flags: review?(jdemooij)
Attachment #8887782 - Flags: review?(jdemooij)
Comment on attachment 8887782 [details] [diff] [review]
optimize-array-push.patch

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

::: js/src/jit/CacheIR.cpp
@@ +3794,5 @@
> +    if (!thisarray->nonProxyIsExtensible())
> +        return false;
> +
> +    // XXX: Is this check required, can args_[0] never be JS_ELEMENTS_HOLE?
> +    if (args_[0].isMagic(JS_ELEMENTS_HOLE))

jandem: was hoping you could answer this for me.  My understanding of HOLEs is that they never leave the array object without being promoted to undefined.. but I've noticed HOLE checks for args in other places, so I'm not sure anymore.
Attachment #8887782 - Flags: review?(jdemooij)
Attached patch optimize-array-push.patch (obsolete) — Splinter Review
Updated patch.

All the stub attach stuff is moved to pre-call, with one call to tryAttachStub.

For StringSplit, this change requires the addition of a TypeMonitor inside the tryAttachStub to record the group of the resulting array in the stack type set.
Attachment #8887782 - Attachment is obsolete: true
Attachment #8888924 - Flags: review?(jdemooij)
Assignee: nobody → kvijayan
Comment on attachment 8888924 [details] [diff] [review]
optimize-array-push.patch

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

Looks good but some questions below.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +2158,3 @@
>          break;
> +      default:
> +        MOZ_ASSERT_UNREACHABLE("Invalid BaselineCacheIRStubKind.");

Nit: convention is to leave out the default case so compilers warn/error when we add new enum values.

::: js/src/jit/BaselineIC.h
@@ +742,4 @@
>  {
>      friend class ICStubSpace;
>    public:
> +    static const unsigned UNOPTIMIZABLE_CALL_FLAG       = (0x1 << 0);

All changes to this file can be reverted I think?

::: js/src/jit/CacheIR.cpp
@@ +3664,5 @@
>      OptStrategy strategy;
> +
> +    // Check for native-function optimizations.
> +    if (calleeFunc->isNative()) {
> +        if ((strategy = canOptimizeStringSplit(calleeFunc)) != OptStrategy::None) {            

Nit: trailing whitespace, no {}

@@ +3717,5 @@
> +    if (thisobj->group()->maybePreliminaryObjects())
> +        return OptStrategy::None;
> +
> +    // Check for other indexed properties or class hooks.
> +    if (!CanAttachAddElement(thisobj, /* isInit = */ false))

Why do we have to check CanAttachAddElement here and in tryAttachArrayPush?

::: js/src/jit/ICState.h
@@ +66,4 @@
>      size_t numOptimizedStubs() const { return numOptimizedStubs_; }
>  
>      MOZ_ALWAYS_INLINE bool canAttachStub() const {
> +        // Note, we cannot assert that numOptimizedStubs_ <= MaxOptimizedStubs because

Nit: wrap comments to 80 columns here and below
(In reply to Jan de Mooij [:jandem] from comment #8)
> Comment on attachment 8888924 [details] [diff] [review]
> optimize-array-push.patch
> Why do we have to check CanAttachAddElement here and in tryAttachArrayPush?

CanAttachAddElement ensures that there are no indexed properties on the proto chain.

|array.push(x)| is largely equivalent to |array[array.length] = x|, which performs the
same length-incrementing, hole-filling behaviour of ArrayPush.  If an indexed accessor
exists on |Array.prototype|, then a push that sets that index will call the accessor.
(In reply to Kannan Vijayan [:djvj] from comment #9)
> > Why do we have to check CanAttachAddElement here and in tryAttachArrayPush?
> 
> CanAttachAddElement ensures that there are no indexed properties on the
> proto chain.

I know, but I meant we were calling it twice.
Attached patch optimize-array-push.patch (obsolete) — Splinter Review
Updated patch, comments addressed.  Passsing reftests and jit-tests on my machine with no failures (timeouts on debug build, but nothing else).

Try run is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68922dd2efe477dae56da07005174d81c800605c&selectedJob=117125486

That try run seemed to have some decider failure, so i ran it again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aab09318af0628306fc83f15b7a30af0243c0d6
Attachment #8888924 - Attachment is obsolete: true
Attachment #8888924 - Flags: review?(jdemooij)
Attachment #8889598 - Flags: review?(jdemooij)
We fail the stubFields_.empty() assert in CacheIRWriter::trace. The easiest fix is probably to scope |gen| better in the fallback code - that also avoids overhead from constructing it when we're not attaching stubs.

Btw I still don't understand why we need both canOptimizeArrayPush and tryAttachArrayPush, wasn't the new plan to merge them both into tryAttachArrayPush? We didn't need the OptStrategy mechanism for the other ICs and I'd really like to keep things as simple as possible - a single tryAttachFoo for each Foo we optimize.
I had thought it was sufficient to just hide that under the 'tryAttach' call instead of actually eliminating the split between the can-optimize, and do-optimize logic.  There's some little pressure inside my head that keeps pushing me towards that preserving that design.

It may be because the check code between different stubs has a bunch of shared logic.  (e.g. checking if the callee is a native function object is shared between both stubs).
Updated patch.  Comments addressed.  Opt and debug builds pass reftests and jit-tests on linux64.  Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebcd9d243b45006a5b4d0c572527f5c687bfc349
Attachment #8889598 - Attachment is obsolete: true
Attachment #8889598 - Flags: review?(jdemooij)
Attachment #8889899 - Flags: review?(jdemooij)
Comment on attachment 8889899 [details] [diff] [review]
optimize-array-push.patch

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

Great, thanks!
Attachment #8889899 - Flags: review?(jdemooij) → review+
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb170d70875
Add CacheIR stub for optimizing calls to array_push.  r=jandem
https://hg.mozilla.org/mozilla-central/rev/5bb170d70875
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.