718683 - IonMonkey: Add a generic GETELEM instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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 [:bzbarsky]]

Does the fast path cover typed arrays too?

Comment 2

[Jan de Mooij [:jandem]]

(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 [:bzbarsky]]

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]]

(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]]

Attachment: 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]]

Attachment: Patch

Comment 7

[Jan de Mooij [:jandem]]

Attachment: 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]]

Attachment: Patch for inbound

Comment 9

[Brian Hackett [Laid off!]]

(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]]

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]]

Attachment: 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]]

Attachment: 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]]

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]]

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]]

https://hg.mozilla.org/mozilla-central/rev/dc059f35f32b

Leaving open for comment 14.

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Patch

I can land this after the next m-c -> ion merge.

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

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]]

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]]

(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.