Last Comment Bug 718683 - IonMonkey: Add a generic GETELEM instruction
: IonMonkey: Add a generic GETELEM instruction
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jan de Mooij [:jandem]
:
:
Mentors:
Depends on:
Blocks: 684381 718982
  Show dependency treegraph
 
Reported: 2012-01-17 08:45 PST by Jan de Mooij [:jandem]
Modified: 2012-01-27 01:29 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (15.96 KB, patch)
2012-01-18 03:54 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch (15.98 KB, patch)
2012-01-18 04:34 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch for inbound (13.33 KB, patch)
2012-01-19 03:49 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch for inbound (13.36 KB, patch)
2012-01-19 03:59 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch for inbound v2 (15.67 KB, patch)
2012-01-21 02:14 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch for inbound v3 (15.52 KB, patch)
2012-01-23 13:45 PST, Jan de Mooij [:jandem]
nicolas.b.pierron: review+
Details | Diff | Splinter Review
Patch (11.10 KB, patch)
2012-01-25 11:49 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-01-17 08:45:38 PST
We have a fast path for dense arrays - everything else should go through a VM call until we have a GETELEM IC.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-01-17 08:59:44 PST
Does the fast path cover typed arrays too?
Comment 2 Jan de Mooij [:jandem] 2012-01-17 09:56:45 PST
(In reply to Boris Zbarsky (:bz) from comment #1)
> Does the fast path cover typed arrays too?

Not yet, we're focusing on the major benchmarks for now, and these don't use typed arrays.. However, at some point we will definitely have to add fast paths for typed arrays to be competitive - the VM call I'm going to add here is a slow fallback to get rid of some compiler aborts and improve test coverage, but it's not going to be fast.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-01-17 10:10:45 PST
Where "major" == "V8 and Sunspider"?  (Not judging; just trying to understand what pieces I need to worry about going forward.)
Comment 4 Jan de Mooij [:jandem] 2012-01-17 10:36:37 PST
(In reply to Boris Zbarsky (:bz) from comment #3)
> Where "major" == "V8 and Sunspider"? 

Sunspider, V8 and Kraken. (Although I'm mostly testing SS for now since most of its tests are relatively small (easier to analyze/debug) and V8 still has some correctness issues.)

It would be good though to look at all performance bugs we used for TM/JM/TI. I'm planning to go through them once we're doing well on SS/V8/Kraken.
Comment 5 Jan de Mooij [:jandem] 2012-01-18 03:54:28 PST
Created attachment 589461 [details] [diff] [review]
Patch

Adds MCallGetElement, like MCallGetProperty etc. Both inputs are Value's since both objects and strings are pretty common. That's also why I used lhs/rhs instead of object/index.

I can land js::GetElement on mozilla-central first to prevent merge pain (this function is huge).
Comment 6 Jan de Mooij [:jandem] 2012-01-18 04:34:53 PST
Created attachment 589465 [details] [diff] [review]
Patch
Comment 7 Jan de Mooij [:jandem] 2012-01-19 03:49:40 PST
Created attachment 589825 [details] [diff] [review]
Patch for inbound

Add GETELEM/SETELEM stubs to jsinterp, I'd like to land this on inbound first. When this merges to the Ion branch we'll have to change a few things to get the right script/pc.
Comment 8 Jan de Mooij [:jandem] 2012-01-19 03:59:08 PST
Created attachment 589826 [details] [diff] [review]
Patch for inbound
Comment 9 Brian Hackett (:bhackett) 2012-01-19 10:55:36 PST
(In reply to Jan de Mooij (:jandem) from comment #5)
> Adds MCallGetElement, like MCallGetProperty etc. Both inputs are Value's
> since both objects and strings are pretty common. That's also why I used
> lhs/rhs instead of object/index.

Does the string case just come up when accessing knownString[n]?  If so, could we add a fast path for that case and allow the generic GETELEM path to only work on objects?  (Will make integrating this into a known-object GETELEM IC easier).
Comment 10 Jan de Mooij [:jandem] 2012-01-19 11:10:22 PST
Comment on attachment 589826 [details] [diff] [review]
Patch for inbound

(In reply to Brian Hackett (:bhackett) from comment #9)
> 
> Does the string case just come up when accessing knownString[n]?  If so,
> could we add a fast path for that case and allow the generic GETELEM path to
> only work on objects?  (Will make integrating this into a known-object
> GETELEM IC easier).

That's fine with me. We won't be able to handle the polymorphic case where both objects and strings are used, but that's probably rare anyway. I'll update the patch tomorrow.
Comment 11 Jan de Mooij [:jandem] 2012-01-21 02:14:43 PST
Created attachment 590451 [details] [diff] [review]
Patch for inbound v2

Splits GetElement in two functions to handle both Value and JSObject. The lazy arguments case still accesses fp and script directly, but we're not going to use lazy args in Ion. Also moves these functions to jsinterpinlines, I think it's important not to regress interpreter performance for relatively common ops.
Comment 12 Jan de Mooij [:jandem] 2012-01-23 13:45:24 PST
Created attachment 590856 [details] [diff] [review]
Patch for inbound v3

Canceled the previous r? request because I wanted to make a few small changes. Patch was green on try (ignoring the unrelated orange).
Comment 13 Nicolas B. Pierron [:nbp] 2012-01-24 05:54:38 PST
Comment on attachment 590856 [details] [diff] [review]
Patch for inbound v3

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

Looks good.

r+ with removal of GetElement and SetElement functions from jsinterp.h which is going to the next patch I guess.

::: js/src/jsinterp.h
@@ +328,5 @@
>  extern bool
>  IsActiveWithOrBlock(JSContext *cx, JSObject &obj, uint32_t stackDepth);
>  
> +bool
> +GetElement(JSContext *cx, const Value &lref, const Value &rref, Value *res);

Ion Monkey patch ?

::: js/src/jsinterpinlines.h
@@ +745,5 @@
> +{
> +    JSScript *script;
> +    jsbytecode *pc;
> +    types::TypeScript::GetPcScript(cx, &script, &pc);
> +    types::TypeScript::MonitorAssign(cx, script, pc, obj, id, value);

Be aware that ion::GetPCScript is slow because it has to recover the FrameInfo and read a snapshot and this is worst in case of inline frames.
Comment 14 Jan de Mooij [:jandem] 2012-01-24 08:37:35 PST
Pushed with nit addressed:

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

Please don't close this bug when merging to m-c, I still have to fix the IonMonkey part.
Comment 15 Ed Morley [:emorley] 2012-01-25 07:26:06 PST
https://hg.mozilla.org/mozilla-central/rev/dc059f35f32b

Leaving open for comment 14.
Comment 16 Jan de Mooij [:jandem] 2012-01-25 11:49:44 PST
Created attachment 591571 [details] [diff] [review]
Patch

I can land this after the next m-c -> ion merge.
Comment 17 David Anderson [:dvander] 2012-01-26 11:52:03 PST
Comment on attachment 591571 [details] [diff] [review]
Patch

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

::: js/src/ion/IonBuilder.cpp
@@ +3062,5 @@
> +        return false;
> +
> +    types::TypeSet *barrier = oracle->propertyReadBarrier(script, pc);
> +    types::TypeSet *types = oracle->propertyRead(script, pc);
> +    return pushTypeBarrier(ins, types, barrier);

I doubt this needs to change, but I'd like to know whether the type barrier here is actually necessary. I think it will run in C++, so the JIT'd barrier will always succeed. That means you could probably pass NULL for |barrier|, or just leave the pushed Value and rely on the type analysis to unbox it where needed.
Comment 18 Jan de Mooij [:jandem] 2012-01-26 15:45:32 PST
http://hg.mozilla.org/projects/ionmonkey/rev/f59064acba0a

(In reply to David Anderson [:dvander] from comment #17)
> 
> I doubt this needs to change, but I'd like to know whether the type barrier
> here is actually necessary. I think it will run in C++, so the JIT'd barrier
> will always succeed. That means you could probably pass NULL for |barrier|,
> or just leave the pushed Value and rely on the type analysis to unbox it
> where needed.

We discussed this on IRC, tomorrow I'll file a follow-up bug to investigate this.
Comment 19 Jan de Mooij [:jandem] 2012-01-27 01:29:13 PST
(In reply to Jan de Mooij (:jandem) from comment #18)
> 
> We discussed this on IRC, tomorrow I'll file a follow-up bug to investigate
> this.

Filed bug 721662.

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