Jason Orendorff [:jorendorff] Reporter
	Description • 10 years ago

Currently, obj[sym] always misses in the Baseline GetElem IC.

	Jason Orendorff [:jorendorff] Reporter
	Comment 1 • 10 years ago

I spent some time blundering around in the code pursuing this, and my latest plan, the one that seemed like it could work, was to turn most of the ICGetElem*Stub classes into templates that parameterize on js::PropertyName vs. JS::Symbol.

Before that, I had hoped to just switch to jsid, but that meant that the stub code would have to unbox the value and rebox it as a jsid -- which seemed like it was going to be slow.

	Jason Orendorff [:jorendorff] Reporter
	Comment 2 • 10 years ago

Er, efaust suggested the templates. At first I was put off by the code duplication, but I'm over it.

	Johannes Schulte [:jschulte] Assignee
	Comment 3 • 10 years ago

Well, that's, I guess, the most straight forward way to do this. Passes tests and gives me a 10x speed-up on microbenchmarking. Not sure, whether there is/we want to take a more elaborate approach? Especially being able to avoid splitting up the ICStub::Kinds would definitely reduce the amount of necessary changes, but I couldn't come up with good way to do this, wdyt?
Should I f? jorendorff, too?

FWIW, I haven't checked style, whitespace etc yet.

	Kannan Vijayan [:djvj]
	Comment 4 • 10 years ago

(In reply to Johannes Schulte [:jschulte] from comment #3)
> Created attachment 8529943 [details] [diff] [review]
> WIP.patch
> 
> Well, that's, I guess, the most straight forward way to do this. Passes
> tests and gives me a 10x speed-up on microbenchmarking. Not sure, whether
> there is/we want to take a more elaborate approach? Especially being able to
> avoid splitting up the ICStub::Kinds would definitely reduce the amount of
> necessary changes, but I couldn't come up with good way to do this, wdyt?
> Should I f? jorendorff, too?
> 
> FWIW, I haven't checked style, whitespace etc yet.

There is a better mechanism for avoiding the kind() explosion.  The key is that we want different stubcode for stubs, but have the kind be the same.  This is fine, because each stub compiler has a virtual function that produces the hashcode used to look up the stubcode for a stub being compiled.  This hash can be computed from both the kind_ as well as other bits of information available to the stub compiler.

You should be able to generate stubs wit the same kind, but have the lookup-code include an additional bit specifying symbol vs. string.

For an example of how this is done, you can take a look at the implementation of ICSetProp_Native, for example.  Notice how it overrides Compiler::getKey with a method that includes the boolean |isFixedSlot_| into the key.  This means that ICSetProp_Native stubs with isFixedSlot_ set will have their own stubcode generated, separate from the stubs with isFixedSlot_=false.

	Johannes Schulte [:jschulte] Assignee
	Comment 5 • 10 years ago

So, this version works without adding new stubkinds. Please have a look, whether this is going in the right direction or someone else should work on this.

	Kannan Vijayan [:djvj]
	Comment 6 • 10 years ago

Comment on attachment 8544158 [details] [diff] [review]
WIPv2.patch

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

Yeah, this is looking pretty decent.  I just did a quick scan of most of the code, and structurally it looks like a good approach.  Fix up the patch, address comments, test, and post again and we should be able to get towards landing soon.

Good job on this.. overall a very nice approach, and this seems like it would have been a bit tedious to code.  Cheers!

Btw, there are a bunch of whitespaces on blank lines and at end-of-lines, too, but I didn't note all of those.  Please do fix them.

::: js/src/jit/BaselineIC.cpp
@@ +3687,5 @@
>              continue;
>          }
>  
> +
> +        ICGetElemNativeStubImpl<T> *getElemNativeStub =

In GetElemNativeStubExists, before this code shows up at all, you need to confirm that T matches the expected type, so something along the lines of..

if (mozilla::Same<T, Symbol>() != ((GetElemNativeStub *) *iter)->isSymbol())
    continue;

That way, the subsequent casts to the templated types are all valid.

@@ +3752,5 @@
>            default:
>              continue;
>          }
>  
> +        ICGetElemNativeStubImpl<T> *getElemNativeStub =

See comment for |GetElemNativeStubExists|

@@ +3831,5 @@
>      return false;
>  }
>  
> +template <class T>
> +T getKey(jsid id)

Nit: declare this |static| scoped.

@@ +3835,5 @@
> +T getKey(jsid id)
> +{MOZ_ASSERT_UNREACHABLE("Key has to be PropertyName or Symbol"); return false;}
> +
> +template <>
> +JS::Symbol *getKey(jsid id)

Nit, for clarify, include the specialized type in the declaration:

template <>
JS::Symbol *getKey<JS::Symbol *>(jsid id)

@@ +3843,5 @@
> +    return JSID_TO_SYMBOL(id);
> +}
> +
> +template <>
> +PropertyName *getKey(jsid id)

Same here.

@@ +3852,5 @@
> +    return JSID_TO_ATOM(id)->asPropertyName();
> +}
> +
> +template <class T>
> +bool checkAtomize(HandleValue key)

Nit: declare |static|

::: js/src/jit/BaselineIC.h
@@ +594,5 @@
>  #undef DEF_ENUM_KIND
> +        GetElem_NativeSlot,
> +        GetElem_NativePrototypeSlot,
> +        GetElem_NativePrototypeCallNative,
> +        GetElem_NativePrototypeCallScripted,

Why are these moving out of the #define macro and directly into the enum?

	Johannes Schulte [:jschulte] Assignee
	Comment 7 • 9 years ago

(In reply to Kannan Vijayan [:djvj] from comment #6)

Well, sorry for the delay, school and another bug kept me busy, but here we go again :)
> ::: js/src/jit/BaselineIC.h
> @@ +594,5 @@
> >  #undef DEF_ENUM_KIND
> > +        GetElem_NativeSlot,
> > +        GetElem_NativePrototypeSlot,
> > +        GetElem_NativePrototypeCallNative,
> > +        GetElem_NativePrototypeCallScripted,
> 
> Why are these moving out of the #define macro and directly into the enum?
Much code is generated from the IC_STUB_KIND_LIST, which would break, if those kinds were in it, e.g. forward declarations(FORWARD_DECLARE_STUBS) and method-definitions(KIND_METHODS). I'm not sure, whether the is a more elegant way to deal with this?

	Kannan Vijayan [:djvj]
	Comment 8 • 9 years ago

Comment on attachment 8588329 [details] [diff] [review]
v1.patch

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

Once again, patch looks reasonable and structurally sound, but I still have some concern about leaf stub types themselves becoming templatized.  That said, it bothers me but I'm willing to let it through once we've talked out some alternatives and decided that this is the best route.

So.. what do you think about having new leaf types that inherit from the templatized ones, and give each variant it's own typename.  This would enable moving the typenames back into the ICSTUB_KIND_LIST instead of breaking them out.

To be more precise, your patch currently defines:

GetElem_NativeSlot<T>
GetElem_NativePrototypeSlot<T>
GetElem_NativePrototypeCallNative<T>
GetElem_NativePrototypeCallScripted<T>

What do you think about defining types:

GetElem_NativeSlotName : public GetElem_NativeSlot<PropertyName *>
GetElem_NativeSlotSymbol : public GetElem_NativeSlot<Symbol *>

GetElem_NativePrototypeSlotName : public GetElem_NativePrototypeSlot<PropertyName *>
GetElem_NativePrototypeSlotSymbol : public GetElem_NativePrototypeSlot<Symbol *>

.. and so forth?

Almost all of the current patch would remain exactly the same, except for the code that instantiates new stubs (within the Compiler classes), which would need to use a helper-method that is templated T, and has two implementations that uses the different names for the stub types.

I'd like your thoughts on whether that would be cleaner, or more complicated and ugly, than what you have here.  The moving-things-out-of-ICSTUB_KIND_LIST issue bothers me because it feels like it might lead to headaches later as the set of stubs we have to move out of that list expands as we make more things Symbol-compatible.  If we end up down the line with most of the element ops (getelem, setelem, in) being pulled out IC_STUB_KIND_LIST, is it going to make life difficult for future devs?

Also, I'm feedback?ing jandem as I'd like to weigh in on this issue.

That concern aside, I'm prepared to r+ this.

	Jan de Mooij [:jandem]
	Comment 9 • 9 years ago

Comment on attachment 8588329 [details] [diff] [review]
v1.patch

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

Sorry for the delay.

(In reply to Kannan Vijayan [:djvj] from comment #8)
> The moving-things-out-of-ICSTUB_KIND_LIST issue bothers me because it feels like
> it might lead to headaches later as the set of stubs we have to move out of
> that list expands as we make more things Symbol-compatible.

Agreed, it'd be good to avoid this.

Johannes, I didn't look too closely at the patch but maybe you can try what Kannan suggested for one or two stubs and see if/how it works?

	Kannan Vijayan [:djvj]
	Comment 10 • 9 years ago

Comment on attachment 8588329 [details] [diff] [review]
v1.patch

Cancelling review to wait for response on this.

	Johannes Schulte [:jschulte] Assignee
	Comment 11 • 9 years ago

Sorry for the delay, hope this is, what you meant.

	Kannan Vijayan [:djvj]
	Comment 12 • 9 years ago

Comment on attachment 8622748 [details] [diff] [review]
v2.patch

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

Looks good, Johannes.  Very sorry for the review lag.  The work week last week cut into things.

You've done an incredible job with this patch.  Thanks for following up on my review comments and going back and addressing issues on this several times.  It has no doubt been tedious.  I appreciate you sticking with it.

	Johannes Schulte [:jschulte] Assignee
	Comment 13 • 9 years ago

Sorry Kannan, but rebasing over bug 1171405 added quite some new code, so I'd feel definitively more comfortable, if you took another quick look.

	Kannan Vijayan [:djvj]
	Comment 14 • 9 years ago

Comment on attachment 8634965 [details] [diff] [review]
v3.patch

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

It's not necessary to do a full review for rebases.  If this passes tests on try, feel free to land it.  Do you have try access?

	Johannes Schulte [:jschulte] Assignee
	Comment 15 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c79630ecd5b4

	Johannes Schulte [:jschulte] Assignee
	Comment 16 • 9 years ago

Well, then thanks for your trust, I guess :)
Yup.

	Johannes Schulte [:jschulte] Assignee
	Comment 17 • 9 years ago

Fixed and rebased patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8599da06265

	Pulsebot
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4960f6c56e

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1a4960f6c56e