Closed Bug 939115 Opened 6 years ago Closed 6 years ago

Optimize arr.splice with unused return value

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jandem, Assigned: isk)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

Many scripts use splice to delete elements from an array, for instance to delete the first 10 elements:

    arr.splice(0, 10);

splice returns an array of deleted elements, but in many cases this array is not used at all. Ion could specialize this and call a different function that does not create the array if the op following the JSOP_CALL is a JSOP_POP.

Peacekeeper's arrayWeighted test does this and both pdf.js and jQuery have many similar splice calls, so it's a valid optimization I think.
Oh unfortunately pdf.js has a bunch of these calls that also add new elements to the array:

    charstring.splice(i, 3, 28, command >> 8, command & 0xff);

This makes it a bit harder but maybe we could support up to 4 extra arguments to handle the most common cases.
I'm interested in to fix this bug.
But I think my knowledge is lack to fix it.
Would you mind helping me to fix this bug?

How can I use gdb in ion-eager mode?
I'd like to know how call array_splice from Ion.
I can get backtrace by brute force.
#0  array_splice (cx=0x10160d740, argc=2, vp=0x7fff5fbfe4c8) at jsarray.cpp:2368
#1  0x000000010178f10c in ?? ()
#2  0x0000000102701558 in ?? ()
#3  0x0000000101783723 in ?? ()
#4  0x00000001001441b2 in EnterBaseline (cx=0x10160d740, data=@0x10234ea00) at BaselineJIT.cpp:122
#5  0x00000001001446b0 in js::jit::EnterBaselineAtBranch (cx=0x10160d740, fp=0x1018b9c30, pc=<value temporarily unavailable, due to optimizations>) at BaselineJIT.cpp:207
#6  0x00000001004ae4c1 in Interpret (cx=0x10160d740, state=@0x7fff5fbff288) at Interpreter.cpp:1569
#7  0x00000001004ad331 in js::RunScript (cx=0x10160d740, state=@0x7fff5fbff288) at Interpreter.cpp:420
#8  0x00000001004bc6d2 in js::ExecuteKernel (cx=0x10160d740, scopeChainArg=@0x10233e060, thisv=@0x7fff5fbff390, type=js::EXECUTE_GLOBAL, result=0x7fff5fbff288) at Interpreter.cpp:611
#9  0x00000001004bc8fb in js::Execute (cx=0x10160d740, scopeChainArg=<value temporarily unavailable, due to optimizations>, rval=0x7fff5fbff5c8) at Interpreter.cpp:647
#10 0x000000010032cd93 in JS_ExecuteScript (cx=0x10160d740, objArg=<value temporarily unavailable, due to optimizations>, scriptArg=<value temporarily unavailable, due to optimizations>, rval=0x7fff5fbff5c8) at jsapi.cpp:4761
#11 0x00000001000074ef in Process (cx=0x10160d740, obj_=<value temporarily unavailable, due to optimizations>, filename=<value temporarily unavailable, due to optimizations>, forceTTY=<value temporarily unavailable, due to optimizations>) at js.cpp:434
#12 0x000000010000411d in main (argc=1, argv=<value temporarily unavailable, due to optimizations>, envp=0x10160d740) at js.cpp:5535

It include ??. So I can't identify how to call array_splice from Ion.
Would you tell me where I should see to fix this bug?
Presumably it gets called right via an LCallNative.
Also affects Browsermark's arrayWeighted, since it appears to be roughly the same code as in Peacekeeper.
Blocks: 940817
Ahem, disregard that -- Browsermark changed the code to use the deleted array.
No longer blocks: 940817
(In reply to Boris Zbarsky [:bz] from comment #4)
> Presumably it gets called right via an LCallNative.
Thank you for your advice.

The backtrace in previous comment(in comment #3) is about baseline-jit.
Gradually,I understand that how to call array_splice.
But I can't understand where I should branch to call array_splice without return value.

I found MCall::New is called in IonBuilder::makeCall and jsop_call sets the function(include native) in following code.

    CallInfo callInfo(alloc(), constructing);
    if (!callInfo.init(current, argc))
        return false;

I research the backtrace about ion and its result is below.

bool
IonBuilder::inspectOpcode(JSOp op)
↓
bool
IonBuilder::jsop_call(uint32_t argc, bool constructing)
↓
bool
IonBuilder::makeCall(JSFunction *target, CallInfo &callInfo, bool cloneAtCallsite)
I _think_ most of the special-casing for calls happens in MCallOptimize.cpp.  But I suggest asking on IRC in #jsapi or #ionmonkey; I'm not a JIT expert, and the folks there are.
(In reply to Boris Zbarsky [:bz] from comment #8)
> I _think_ most of the special-casing for calls happens in MCallOptimize.cpp.

Yes, we should add a new MArraySplice/LArraySplice instruction and add it in MCallOptimize.cpp, then add a new C++ function that instruction can call.

Feel free to take this bug, but it's fairly complicated if you're new to the JITs..
(In reply to Jan de Mooij [:jandem] from comment #9)
> (In reply to Boris Zbarsky [:bz] from comment #8)
> > I _think_ most of the special-casing for calls happens in MCallOptimize.cpp.
> 
> Yes, we should add a new MArraySplice/LArraySplice instruction and add it in
> MCallOptimize.cpp, then add a new C++ function that instruction can call.
> 
> Feel free to take this bug, but it's fairly complicated if you're new to the
> JITs..

Thank you for your comment.

I'd like to take it bug.
But I don't have enough knowledge about JIT.
Could you advise me as a mentor?
Assignee: nobody → iseki.m.aa
Flags: needinfo?(jdemooij)
I make MArraySplice/LArraySplice instruction.
This patch is corresponding to array_slice which returns objects.
Previous patch has many compiler error.
It is resolved in this patch.
Attachment #8339860 - Attachment is obsolete: true
(In reply to iseki.m.aa from comment #11)
> I make MArraySplice/LArraySplice instruction.
> This patch is corresponding to array_slice which returns objects.

Cool, I think MArraySplice and LArraySplice is the way to go. We should also add a C++ function that takes an object and uint32_t deleteCount and does the splice part without returning a new array. Then CodeGenerator::visitArraySplice can call this function.

I'd be happy to mentor you, but this bug is pretty complicated. It may be better to fix some simpler JIT bugs first :)
Flags: needinfo?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to iseki.m.aa from comment #11)
> > I make MArraySplice/LArraySplice instruction.
> > This patch is corresponding to array_slice which returns objects.
> 
> Cool, I think MArraySplice and LArraySplice is the way to go. We should also
> add a C++ function that takes an object and uint32_t deleteCount and does
> the splice part without returning a new array. Then
> CodeGenerator::visitArraySplice can call this function.
> 
> I'd be happy to mentor you, but this bug is pretty complicated. It may be
> better to fix some simpler JIT bugs first :)

Thanks Jan :)
I think bug848510 is simpler than this bug.
So I will try fixing bug848510 first.
Status: NEW → ASSIGNED
I'm sorry for late, i try another bug.
I have a few question.

1. This bug is still worth if Bug890329 is resolved?
2. The mean of a C++ function(Comment 13) is VMFunction?
3. How to identify JSOP_POP in MCallOptimize.cpp?
Flags: needinfo?(jdemooij)
(In reply to Masaya Iseki[:isk](UTC+9) from comment #15)
> 1. This bug is still worth if Bug890329 is resolved?

Yes I think it is. If we self-host splice, we'll still create the result array. We'd need an analysis to determine the array is not used, but we don't have that yet.

> 2. The mean of a C++ function(Comment 13) is VMFunction?

Yep, you could add a C++ function like arr_splice that does not create/return the array.

> 3. How to identify JSOP_POP in MCallOptimize.cpp?

You can call BytecodeIsPopped(pc) to check if the JSOP_CALL is followed by a JSOP_POP.

Bug 958713 also has some info btw.
Flags: needinfo?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #16)
> > 3. How to identify JSOP_POP in MCallOptimize.cpp?
> You can call BytecodeIsPopped(pc) to check if the JSOP_CALL is followed by a
> JSOP_POP.

CallResultEscapes() does this.
(In reply to Sean Stangl [:sstangl] from comment #17)
> > You can call BytecodeIsPopped(pc) to check if the JSOP_CALL is followed by a
> > JSOP_POP.
> 
> CallResultEscapes() does this.

That one also allows if (arr.splice(..)) and if (!arr.splice(..) though, so you have to be more careful to make the splice function return |true| or something in this case. Since splice always returns an object that should work but BytecodeIsPopped seems simpler.
Attached patch (WIP) Add instruction.patch (obsolete) — Splinter Review
This patch cause following assertion failure when run the benchmark(I'll upload next comment).

 Assertion failure: shape_, at /Users/iseki/Documents/source/mozilla-central/js/src/vm/ObjectImpl.h:1183

I don't know the reason of this failure.
Would you tell me?


Following log is the backtrace of gdb.

#0  js::jit::AssertValidObjectPtr (cx=<value temporarily unavailable, due to optimizations>, obj=<value temporarily unavailable, due to optimizations>) at ObjectImpl.h:1183
#1  0x00000001020ef574 in ?? ()
#2  0x0000000102505648 in ?? ()
#3  0x00000001020d3723 in ?? ()
#4  0x00000001001b495a in EnterBaseline (cx=0x0, data=@0x0) at BaselineJIT.cpp:122
#5  0x00000001001b42b4 in js::jit::EnterBaselineMethod (cx=0x102200790, state=@0x7fff5fbfc4a8) at BaselineJIT.cpp:153
#6  0x00000001004d4f9d in js::RunScript (cx=<value temporarily unavailable, due to optimizations>, state=<value temporarily unavailable, due to optimizations>) at Interpreter.cpp:410
#7  0x00000001004e49d4 in js::Invoke (cx=0x102200790, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x7fff5fbfc6e0}, <No data fields>}, argc_ = 2}, <No data fields>}, construct=<value temporarily unavailable, due to optimizations>) at Interpreter.cpp:483
#8  0x00000001004c058e in js::Invoke (cx=0x102200790, thisv=<value temporarily unavailable, due to optimizations>, fval=<value temporarily unavailable, due to optimizations>, argc=<value temporarily unavailable, due to optimizations>, argv=0x7fff5fbfcce0) at Interpreter.cpp:520
#9  0x00000001001ac530 in js::jit::DoCallFallback (cx=<value temporarily unavailable, due to optimizations>, frame=0x7fff5fbfcd30, stub=0x102506c58, argc=2, vp=0x7fff5fbfccd0) at BaselineIC.cpp:8097
#10 0x00000001020db73b in ?? ()
#11 0x0000000102506c58 in ?? ()
Attachment #8339867 - Attachment is obsolete: true
Attachment #8380341 - Flags: feedback?(jdemooij)
Attached file array_splice_bench.js
Attached patch (WIP)Add instruction.patch (obsolete) — Splinter Review
I found the cause of SEGV.
It is the return value of VMFunction. I change it to JSObject from bool.

But I have other problem. This patch can't erase element in Ion. 
For example:

var res = [1,2,3];
res.splice(0,2);
print(res);

The correct result is [3] , but [1,2,3] in this patch.
Attachment #8380341 - Attachment is obsolete: true
Attachment #8380341 - Flags: feedback?(jdemooij)
Attachment #8388188 - Flags: feedback?(jdemooij)
Attached patch Bug939115.patch (obsolete) — Splinter Review
I found the cause of that can't erase elements.
It is because deleteCount and start is always 0.
I modify it.

I measure running time(use array_splice_bench.js).

size is 500000.

make return value: 1435 ms
not make return value: 1415 ms
Attachment #8388188 - Attachment is obsolete: true
Attachment #8388188 - Flags: feedback?(jdemooij)
Attachment #8392052 - Flags: review?(jdemooij)
Comment on attachment 8392052 [details] [diff] [review]
Bug939115.patch

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

Sorry for the delay. Thanks for the patch, looks pretty good! Most comments below are nits.

::: js/src/jit/LIR-Common.h
@@ +1287,5 @@
>          return getTemp(1);
>      }
>  };
>  
> +class LArraySplice : public LCallInstructionHelper<1, 3, 0>

0, 3, 0 -- we're not using the result.

::: js/src/jit/Lowering.cpp
@@ +506,5 @@
> +{
> +    LArraySplice *lir = new(alloc()) LArraySplice(useFixed(ins->object(), CallTempReg0),
> +                                                  useFixed(ins->start(), CallTempReg1),
> +                                                  useFixed(ins->deleteCount(), CallTempReg2));
> +    return defineReturn(lir, ins) && assignSafepoint(lir, ins);

Remove the defineReturn(lir, ins) here, we're not interested in the result in this case.

::: js/src/jit/MCallOptimize.cpp
@@ +386,5 @@
> +    // Ensure |this|, argument and result are objects.
> +    if (getInlineReturnType() != MIRType_Object)
> +        return InliningStatus_NotInlined;
> +    if (callInfo.thisArg()->type() != MIRType_Object)
> +        return InliningStatus_NotInlined;

Also add:

    if (callInfo.getArg(0)->type() != MIRType_Int32)
        return InliningStatus_NotInlined;
    if (callInfo.getArg(1)->type() != MIRType_Int32)
        return InliningStatus_NotInlined;

@@ +387,5 @@
> +    if (getInlineReturnType() != MIRType_Object)
> +        return InliningStatus_NotInlined;
> +    if (callInfo.thisArg()->type() != MIRType_Object)
> +        return InliningStatus_NotInlined;
> +    // |this| and the argument must be dense arrays.

I think you can remove this comment and the code from here...

@@ +413,5 @@
> +    if (!baseThisType)
> +        return InliningStatus_NotInlined;
> +    types::TypeObjectKey *thisType = types::TypeObjectKey::get(baseThisType);
> +    if (thisType->unknownProperties())
> +        return InliningStatus_NotInlined;

..to here, because we don't rely on these checks in this case.

@@ +417,5 @@
> +        return InliningStatus_NotInlined;
> +
> +    callInfo.setImplicitlyUsedUnchecked();
> +
> +    if (!BytecodeIsPopped(pc))

Add a comment above this line:

    // Specialize arr.splice(start, deleteCount) with unused return value and
    // avoid creating the result array in this case.

@@ +424,5 @@
> +    MArraySplice *ins = MArraySplice::New(alloc(),
> +                                          callInfo.thisArg(),
> +                                          callInfo.getArg(0),
> +                                          callInfo.getArg(1)
> +                                          );

Nit: move ); to the end of the previous line.

::: js/src/jit/MIR.h
@@ +1979,5 @@
>  
>      virtual void computeMovable() MOZ_OVERRIDE;
>  };
>  
> +// Array.prototype.splice

// arr.splice(start, deleteCount) with unused return value.

@@ +1981,5 @@
>  };
>  
> +// Array.prototype.splice
> +class MArraySplice : public MTernaryInstruction
> +{

class MArraySplice
  : public MTernaryInstruction,
    public Mix3Policy<ObjectPolicy<0>, IntPolicy<1>, IntPolicy<2> >
{

@@ +1987,5 @@
> +
> +    MArraySplice(MDefinition *object, MDefinition *start, MDefinition *deleteCount)
> +      : MTernaryInstruction(object, start, deleteCount)
> +    {
> +        setResultType(MIRType_Object);

Remove this line, we're not using the return value.

@@ +2012,5 @@
> +    }
> +
> +    bool possiblyCalls() const {
> +        return true;
> +    }

Add this method:

    TypePolicy *typePolicy() {
        return this;
    }

::: js/src/jit/VMFunctions.h
@@ +670,5 @@
>  
>  JSObject *CreateDerivedTypedObj(JSContext *cx, HandleObject descr,
>                                  HandleObject owner, int32_t offset);
>  
> +JSObject *ArraySpliceDense(JSContext *cx, HandleObject obj, uint32_t start, uint32_t deleteCount);

Make this return bool instead of JSObject*.

::: js/src/jsarray.cpp
@@ +2353,5 @@
>  }
>  
>  /* ES5 15.4.4.12. */
> +bool
> +js::array_splice(JSContext *cx, unsigned argc, Value *vp) {

Nit: { on its own line.

@@ +2358,5 @@
> +    return array_splice_can_pop(cx, argc, vp, false);
> +}
> +
> +bool
> +js::array_splice_can_pop(JSContext *cx, unsigned argc, Value *vp, bool pop)

Rename pop to returnValueIsUsed

::: js/src/jsarray.h
@@ +126,5 @@
>  
>  extern bool
> +array_splice(JSContext *cx, unsigned argc, js::Value *vp);
> +extern bool
> +array_splice_can_pop(JSContext *cx, unsigned argc, js::Value *vp, bool pop);

Rename this to: array_splice_impl

And add a new line before |extern bool|
Attachment #8392052 - Flags: review?(jdemooij)
Attached patch Bug939115.patch (obsolete) — Splinter Review
Thank you for your reviewing.

I modify this patch but I have other problem.
As a result of modification according to the comment, this patch fail assertion at JS_ASSERT(isLowered())[1]. I don't know why |isLowered| return false.

I tried to change |assignSafepoint| to |assignSnapshot|. The assertion failure is not occurred but the state of array is not changed before call method (array's element is the same before call |splice|).

I don't know what should I do to get correct result. 
If you don't mind, would you please tell me?

[1]http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#547
Attachment #8392052 - Attachment is obsolete: true
Attachment #8394587 - Flags: feedback?(jdemooij)
Sorry again for the delay. I'll reply faster next time.

(In reply to Masaya Iseki[:isk](UTC+9) from comment #24)
> I modify this patch but I have other problem.
> As a result of modification according to the comment, this patch fail
> assertion at JS_ASSERT(isLowered())[1]. I don't know why |isLowered| return
> false.

I think this line:

return assignSnapshot(lir);

Should be:

return add(lir, ins) && assignSafepoint(lir, ins);

Does that help?
Attachment #8394587 - Flags: feedback?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #25)

Thank you for your answering.

> I think this line:
> 
> return assignSnapshot(lir);
> 
> Should be:
> 
> return add(lir, ins) && assignSafepoint(lir, ins);
> 
> Does that help?

I modify it but this patch fail the same assertion(commnet #24).
Sorry, I still don't understand the lowering scheme well.
The reason that the state of array is not changed is to change |assignSafepoint| to |assignSnapshot|, right?
(In reply to Masaya Iseki[:isk](UTC+9) from comment #26)
> I modify it but this patch fail the same assertion(commnet #24).

Can you post an updated patch that applies to a recent mozilla-inbound/mozilla-central revision?

> The reason that the state of array is not changed is to change
> |assignSafepoint| to |assignSnapshot|, right?

It has to be assignSafepoint, because it's a fallible VM call. assignSnapshot is not necessary.
(In reply to Masaya Iseki[:isk](UTC+9) from comment #26)
> Sorry, I still don't understand the lowering scheme well.

add - Is made to register the register allocation on top of the MIR Instruction.

assignSnapshot - Is made to register the list of allocation captured by the last resume point, such as we can return to baseline safely.

assignSafepoint - Is made to capture the list of allocation at the time of this instruction, such as we can GC live objects which are on the stack / memory.

As the codegen implementation is doing a callVM, you want to have a Safepoint, because the call can fall into a GC.  You do not need a Safepoint as you have no bailout in the Codegen.  You need "add" such as this instruction is visited in the CodeGenerator.
Attached patch Bug939115.patch (obsolete) — Splinter Review
(In reply to Jan de Mooij [:jandem] from comment #27)
This is an updated patch that applies to a recent mozilla-central version.

This patch still fail the same assertion.
Attachment #8394587 - Attachment is obsolete: true
Attachment #8403910 - Flags: feedback?(jdemooij)
(In reply to Nicolas B. Pierron [:nbp] from comment #28)
Thank you for your answering!
Comment on attachment 8403910 [details] [diff] [review]
Bug939115.patch

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

::: js/src/jit/MCallOptimize.cpp
@@ +427,5 @@
> +                                          callInfo.getArg(0),
> +                                          callInfo.getArg(1));
> +
> +    current->add(ins);
> +    current->push(ins);

What happens if you remove current->push(ins); and instead do:

pushConstant(UndefinedValue());

MArraySplice now has MIRType_None (this means it does not return a value), so we shouldn't push it on the stack and use it as input for other instructions.
Attachment #8403910 - Flags: feedback?(jdemooij)
Attached patch Bug939115.patch (obsolete) — Splinter Review
Thanks!
This patch work fine thanks to your comment.

This result is following(size = 100000):

before:5562ms
after:2186ms
Attachment #8403910 - Attachment is obsolete: true
Attachment #8403987 - Flags: review?(jdemooij)
Comment on attachment 8403987 [details] [diff] [review]
Bug939115.patch

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

Thanks! This looks great. Also nice speedup.

Please address the nits below and I'll r+ and land this for you.

::: js/src/jit/Lowering.cpp
@@ +570,5 @@
> +LIRGenerator::visitArraySplice(MArraySplice *ins)
> +{
> +    LArraySplice *lir = new(alloc()) LArraySplice(useFixed(ins->object(), CallTempReg0),
> +                                                  useFixed(ins->start(), CallTempReg1),
> +                                                  useFixed(ins->deleteCount(), CallTempReg2));

Nit: instead of useFixed(), use useRegisterAtStart():

useRegisterAtStart(ins->object()) etc.

::: js/src/jit/VMFunctions.cpp
@@ +372,5 @@
> +    argv[1].setObject(*obj);
> +    argv[2].set(Int32Value(start));
> +    argv[3].set(Int32Value(deleteCount));
> +
> +    if (!js::array_splice_can_impl(cx, 2, argv.begin(), true))

Nit: |false| instead of |true|

Also, you can change this to: return array_splice(...);

::: js/src/jsarray.cpp
@@ +2353,5 @@
>  }
>  
>  /* ES5 15.4.4.12. */
> +bool
> +js::array_splice(JSContext *cx, unsigned argc, Value *vp) {

Nit: { on the next line

@@ +2354,5 @@
>  
>  /* ES5 15.4.4.12. */
> +bool
> +js::array_splice(JSContext *cx, unsigned argc, Value *vp) {
> +    return array_splice_can_impl(cx, argc, vp, false);

This should pass |true| instead of |false|.

@@ +2358,5 @@
> +    return array_splice_can_impl(cx, argc, vp, false);
> +}
> +
> +bool
> +js::array_splice_can_impl(JSContext *cx, unsigned argc, Value *vp, bool returnValueIsUsed)

Nit: rename this to array_splice_impl

@@ +2404,5 @@
>      JS_ASSERT(len - actualStart >= actualDeleteCount);
>  
>      /* Steps 2, 8-9. */
>      Rooted<ArrayObject*> arr(cx);
> +    if (!returnValueIsUsed) {

if (returnValueIsUsed)

@@ +2567,5 @@
>      if (!SetLengthProperty(cx, obj, finalLength))
>          return false;
>  
>      /* Step 17. */
> +    if (!returnValueIsUsed)

Same here.
Attachment #8403987 - Flags: review?(jdemooij)
Attached patch Bug939115.patchSplinter Review
Thank you for your review.
I fix it.
Attachment #8403987 - Attachment is obsolete: true
Attachment #8410669 - Flags: review?(jdemooij)
Comment on attachment 8410669 [details] [diff] [review]
Bug939115.patch

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

Thanks, looks great! I fixed the comments below and added a testcase and pushed this to Try:

https://tbpl.mozilla.org/?tree=Try&rev=8a44e9f4dd24

::: js/src/jsarray.cpp
@@ +2353,5 @@
>  }
>  
>  /* ES5 15.4.4.12. */
> +bool
> +js::array_splice(JSContext *cx, unsigned argc, Value *vp) {

Nit: { on its own line.

@@ +2421,5 @@
> +            for (uint32_t k = 0; k < actualDeleteCount; k++) {
> +                bool hole;
> +                if (!CheckForInterrupt(cx) ||
> +                    !GetElement(cx, obj, actualStart + k, &hole, &fromValue) ||
> +                    (!hole && !JSObject::defineElement(cx, arr, k, fromValue)))

I just realized the GetElement calls are observable because they can call getters, so it's wrong to stop calling them even if returnValueIsUsed is false.

It's probably best to check returnValueIsUsed only in the CanOptimizeForDenseStorage case.
Attachment #8410669 - Flags: review?(jdemooij) → review+
needinfo myself to land this if Try looks good.
Flags: needinfo?(jdemooij)
Try was green so I pushed this:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a922f75bfb18

Thanks again for the patch!
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/a922f75bfb18
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1006065
Duplicate of this bug: 958713
You need to log in before you can comment on or make changes to this bug.