Last Comment Bug 773995 - IonMonkey: Inline js_Array «new Array» with 2 arguments or more.
: IonMonkey: Inline js_Array «new Array» with 2 arguments or more.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on:
Blocks: IonSpeed
  Show dependency treegraph
 
Reported: 2012-07-14 14:28 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-07-19 13:52 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Inline new Array with 2 or more arguments. (5.20 KB, patch)
2012-07-14 14:47 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Inline new Array with 2 or more arguments. (4.82 KB, patch)
2012-07-17 08:27 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Inline new Array with 2 or more arguments. [v +Inf] (5.68 KB, patch)
2012-07-19 10:52 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Inline new Array with 2 or more arguments. [v +2*Inf] (6.26 KB, patch)
2012-07-19 10:57 PDT, Nicolas B. Pierron [:nbp]
jdemooij: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-07-14 14:28:45 PDT
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)
Comment 1 Nicolas B. Pierron [:nbp] 2012-07-14 14:47:30 PDT
Created attachment 642273 [details] [diff] [review]
Inline new Array with 2 or more arguments.

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.
Comment 2 Jan de Mooij [:jandem] (PTO until July 31) 2012-07-16 13:36:28 PDT
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?
Comment 3 Nicolas B. Pierron [:nbp] 2012-07-16 15:18:35 PDT
(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.
Comment 4 Jan de Mooij [:jandem] (PTO until July 31) 2012-07-17 00:27:25 PDT
(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.
Comment 5 Nicolas B. Pierron [:nbp] 2012-07-17 08:25:39 PDT
(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.
Comment 6 Nicolas B. Pierron [:nbp] 2012-07-17 08:27:19 PDT
Created attachment 642966 [details] [diff] [review]
Inline new Array with 2 or more arguments.

Delta:
Remove the effectful flag of MNewArray and update the assertion in callVM.
Comment 7 Jan de Mooij [:jandem] (PTO until July 31) 2012-07-18 01:54:34 PDT
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.
Comment 8 Nicolas B. Pierron [:nbp] 2012-07-19 10:52:53 PDT
Created attachment 643927 [details] [diff] [review]
Inline new Array with 2 or more arguments. [v +Inf]

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?).
Comment 9 Nicolas B. Pierron [:nbp] 2012-07-19 10:57:26 PDT
Created attachment 643932 [details] [diff] [review]
Inline new Array with 2 or more arguments. [v +2*Inf]

Remove extra resume points since NewArray is no longer effectful.
Comment 10 Jan de Mooij [:jandem] (PTO until July 31) 2012-07-19 12:14:53 PDT
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).
Comment 11 Nicolas B. Pierron [:nbp] 2012-07-19 13:52:09 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/88e464ff51b8

Note You need to log in before you can comment on or make changes to this bug.