Closed Bug 773995 Opened 12 years ago Closed 12 years ago

IonMonkey: Inline js_Array «new Array» with 2 arguments or more.

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

new Array is inlined with 0 or 1 arguments.  sunspider 3d-raytrace need «new Array» implementation.  Currently IonMonkey is 8 times slower than JM on

new Array(1, 2, 3)
Make MNewArray non-effectful for the implementation of «new Array(1, 2, 3)» to replace the NEW bytecode by an allocation and the initialization handled in multiple MIR instructions.

In the past we chose to make NewArray effectful to avoid reallocation of large memory areas in case of oom, but in such case the number of argument is limited by the number of stack slot accepted by IonMonkey.

As opposed to JSOP_NEWARRAY / JSOP_INITELEM we do not need to initialize the length of the array after each store-element because we should not have any GC in the middle of the initialization.
Attachment #642273 - Flags: review?(jdemooij)
Assignee: nicolas.b.pierron → general
Component: js-ctypes → JavaScript Engine
Comment on attachment 642273 [details] [diff] [review]
Inline new Array with 2 or more arguments.

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

::: js/src/ion/MCallOptimize.cpp
@@ +191,5 @@
> +        // array. By setting it as non-effectful we will resume before the call
> +        // which can fail again the same allocation.  At the same time inline
> +        // initialization is restricted by the number of local slot, which means
> +        // that we are not making huge allocations here.
> +        effectfulNew = false;

Correct me if I'm wrong but I don't think we need to do this, the StoreElement instructions we add below are always effectful so we will definitely resume after them right?

@@ +237,5 @@
> +            current->add(store);
> +        }
> +
> +        // Update the length.
> +        MSetInitializedLength *length = MSetInitializedLength::New(elements, id);

The last index is < initLength, so if we have new Array(1, 2, 3) this will set the initialized length to 2 instead of 3. Can you add a testcase for this?
Attachment #642273 - Flags: review?(jdemooij)
(In reply to Jan de Mooij (:jandem) from comment #2)
> Comment on attachment 642273 [details] [diff] [review]
> Inline new Array with 2 or more arguments.
> 
> Review of attachment 642273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/MCallOptimize.cpp
> @@ +191,5 @@
> > +        // array. By setting it as non-effectful we will resume before the call
> > +        // which can fail again the same allocation.  At the same time inline
> > +        // initialization is restricted by the number of local slot, which means
> > +        // that we are not making huge allocations here.
> > +        effectfulNew = false;
> 
> Correct me if I'm wrong but I don't think we need to do this, the
> StoreElement instructions we add below are always effectful so we will
> definitely resume after them right?

I did that to work around the assertion of callVM:

   JS_ASSERT_IF(mir->isEffectful(), mir->resumePoint());

But as you make me notice, this is legacy code it should ensure that there is a safepoint instead of a resumepoint.


> @@ +237,5 @@
> > +            current->add(store);
> > +        }
> > +
> > +        // Update the length.
> > +        MSetInitializedLength *length = MSetInitializedLength::New(elements, id);
> 
> The last index is < initLength, so if we have new Array(1, 2, 3) this will
> set the initialized length to 2 instead of 3. Can you add a testcase for
> this?

I checked, SetInitializedLength expect the lasted initialized index.  I cannot remember what I had in mind at the time, but this is likely to avoid the multiplecation of constant.

I extracted this code from jsop_initelem_dense.
Component: JavaScript Engine → js-ctypes
(In reply to Nicolas B. Pierron [:pierron] from comment #3)
> 
> I did that to work around the assertion of callVM:

It should be fine to call resumeAfter to assign a resume point to MNewArray, although it's never used due to the effectful StoreElement immediately after it.
Assignee: general → nicolas.b.pierron
Component: js-ctypes → JavaScript Engine
(In reply to Jan de Mooij (:jandem) from comment #4)
> (In reply to Nicolas B. Pierron [:pierron] from comment #3)
> > 
> > I did that to work around the assertion of callVM:
> 
> It should be fine to call resumeAfter to assign a resume point to MNewArray,
> although it's never used due to the effectful StoreElement immediately after
> it.

I am not sure to understand what this would mean to resume after the MNewArray, because the corresponding Bytecode (JSOP_NEW) instruction would expect the initialization.  At the same time this is not necessary as it is bounded by the stack space.
Delta:
Remove the effectful flag of MNewArray and update the assertion in callVM.
Attachment #642273 - Attachment is obsolete: true
Attachment #642966 - Flags: review?(jdemooij)
Comment on attachment 642966 [details] [diff] [review]
Inline new Array with 2 or more arguments.

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

As discussed yesterday, I think we should treat NewArray as non-effectful, so that we will resume *before* the CALL if NewArray triggers a GC and invalidates the script.
Attachment #642966 - Flags: review?(jdemooij)
Mark NewArray as never effectful, and explain why we can safely do so in a not-so-short comment.

Reuse the same rule/heuristic used in JM to avoid allocating too much inline (unallocated?).
Attachment #642966 - Attachment is obsolete: true
Attachment #643927 - Flags: review?(jdemooij)
Remove extra resume points since NewArray is no longer effectful.
Attachment #643927 - Attachment is obsolete: true
Attachment #643927 - Flags: review?(jdemooij)
Attachment #643932 - Flags: review?(jdemooij)
Comment on attachment 643932 [details] [diff] [review]
Inline new Array with 2 or more arguments. [v +2*Inf]

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

Looks good, r=me with nits addressed.

::: js/src/ion/MCallOptimize.cpp
@@ +187,5 @@
>  
> +        size_t maxArraySlots =
> +            gc::GetGCKindSlots(gc::FINALIZE_OBJECT_LAST) - ObjectElements::VALUES_PER_HEADER;
> +        if (argc > maxArraySlots)
> +            alloc = MNewArray::NewArray_Allocating;

Nit: as discussed on IRC, you may be able to remove this.

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +371,5 @@
>  #ifdef DEBUG
>      if (ins->mirRaw()) {
>          JS_ASSERT(ins->mirRaw()->isInstruction());
>          MInstruction *mir = ins->mirRaw()->toInstruction();
> +        JS_ASSERT_IF(mir->isEffectful(), ins->safepoint());

Nit: I think we should keep the previous assert: if an instruction is effectful, it should have a resumepoint since we won't redo effectful operations in the interpreter (and making NewArray non-effectful avoids the assert).
Attachment #643932 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/88e464ff51b8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.