Inline loading of immutable properties

Inline loading of immutable properties

Adds detection for property loads that are immutable (non-configurable and non-writable) and inlines them where possible.

BUG=
R=verwaest@chromium.org

[p.antonov, 2014-04-02 13:12:37]

Using a simpler and more generic approach in inlining immutable properties like
Math.PI. PTAL.

[Toon Verwaest, 2014-04-02 14:51:46]

Much cleaner!

Could you move the code to BuildNamedAccess though? That's the single bottleneck
where all named property accesses go through.

https://codereview.chromium.org/220163012/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/220163012/diff/1/src/hydrogen.cc#newcode6730
src/hydrogen.cc:6730: (js_object->map()->is_frozen() ||
You don't need to special-case is_frozen(), such properties will be read-only
non-deleteable.

https://codereview.chromium.org/220163012/diff/1/src/hydrogen.cc#newcode6736
src/hydrogen.cc:6736: !value->IsCallable()) {
Why these cases? Can you introduce abstractions that make it clear what these
cases mean?

[p.antonov, 2014-04-02 15:53:58]

https://codereview.chromium.org/220163012/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/220163012/diff/1/src/hydrogen.cc#newcode6736
src/hydrogen.cc:6736: !value->IsCallable()) {
On 2014/04/02 14:51:47, Toon Verwaest wrote:
> Why these cases? Can you introduce abstractions that make it clear what these
> cases mean?

The null check should actually be unnecessary (since the lookup is found?).

About the hole check: I took a look at the different lookup types and it looked
like Normal, Field and Constant were the ones that can be applied in this
situation. I didn't find a method that checks for all of those but I noticed
GetLazyValue returns TheHole if the lookup type is not a normal, a field or a
constant. Should I make a method that checks of the lookup type is one of these
three types?

The callable check is there because if the value is a function, then inlining
its load and calling it causes crash. I think this is because of some assumption
is being violated somewhere but I don't know at this time what it is. Should I
add a fixme comment?

[p.antonov, 2014-04-03 09:35:31]

I moved the code which turned out to be handy in fixing the callable issue as
well:

in VisitCall, shortly after PushLoad is called it is checked if the function is
a HConstant, and if it is, types->first() is called without checking the length
of the types (assumption is that there is at least 1 type).

However when using for example --always-opt this optimization used to return a
constant function when types->length is 0 and this causes a crash when
types->first() is called.

This is fixed by moving the optimization after the check has been done that
types->length() > 0.

[Toon Verwaest, 2014-04-03 11:42:34]

That sounds reasonable.

On second thought, please move all of this code to BuildLoadNamedField. The rest
of the machinery is there to ensure that all checks are done properly, and you
*do* need them as well. Suppose the value you are loading comes from the
prototype chain. We are guaranteed that the value doesn't disappear from its
holder, however, the prototype chain could change; or the property could appear
in between. 

Putting the code in BuildLoadNamedField ensures that all checks are done
properly (and possibly elided) in BuildMonomorphicAccess.

[p.antonov, 2014-04-03 12:26:07]

On 2014/04/03 11:42:34, Toon Verwaest wrote:
> That sounds reasonable.
> 
> On second thought, please move all of this code to BuildLoadNamedField. The
rest
> of the machinery is there to ensure that all checks are done properly, and you
> *do* need them as well. Suppose the value you are loading comes from the
> prototype chain. We are guaranteed that the value doesn't disappear from its
> holder, however, the prototype chain could change; or the property could
appear
> in between. 
> 
> Putting the code in BuildLoadNamedField ensures that all checks are done
> properly (and possibly elided) in BuildMonomorphicAccess.

Doesn't LocalLookup (with the false parameter) ensure that the value to be
loaded
was always on the object itself?

[Toon Verwaest, 2014-04-03 15:41:59]

Yes it does. But why do you limit it to just that case? Why not also support
values that are eg loaded from the prototype chain (and hence predictable) that
are not configurable / read-only?

[p.antonov, 2014-04-03 16:02:05]

On 2014/04/03 15:41:59, Toon Verwaest wrote:
> Yes it does. But why do you limit it to just that case? Why not also support
> values that are eg loaded from the prototype chain (and hence predictable)
that
> are not configurable / read-only?

Originally it was there because of the frozen check which was done on
the object itself only.

The code cannot be moved to BuildLoadNamedField because only Field loads arrive
there
(so it wouldn't work for Math.PI, which is Normal). Can it be moved to
BuildMonomorphicAccess after the check for prototypes have been built?

Also, at this point, in the case the property was directly on the object, isn't
the CheckMaps
that was added BuildNamedAccess just before the call to BuildMonomorphicAccess
unnecessary?

[Toon Verwaest, 2014-04-03 16:14:34]

Yes you are right, the HCheckMaps is unnecessary in the case it's directly on
the object. However, that's also true in case the property is in the prototype
chain. The holder requires no checks, while all objects up to the holder do need
map-checks (and negative lookups in case of NORMAL).

So I think a better solution is to detect whether the holder needs a check, and
eliminate the holder-check in case it's read-only non-deletable. But that's a
separate optimization. I'd (for now) just emit the map-checks.

You are also right about the "normal" object case, I missed that.

[p.antonov, 2014-04-04 13:23:09]

I added support from loading from prototypes which works
even if the object is not a Constant.

If the property is directly on the object then it needs
to be a Constant (e.g. Math).

I also had a go at eliminating the map check from the holder.

[Toon Verwaest, 2014-04-08 11:50:28]

So I think this is going in the right direction. However, this should be split
into 3 patches. There are three independent optimizations at hand.

1) Inlining a constant when the holder (either receiver or a prototype up the
chain of the receiver) is an HConstant and the property is read-only
non-configurable, and
2) omitting the map-check for the holder if the holder is an HConstant and we're
emitting a map-check only as guard for a property load of a read-only,
non-configurable property.
3) What you suggested regarding the types in VisitCall: if the receiver is an
HConstant, we should adjust the types list accordingly.

Lets make 1) this patch, and figure out 2) in a separate patch; and land 3)
first to make sure that we don't have problems in 1).

My suggestions for the patches:

1) Move the code inlining the HConstant into BuildLoadNamedField if
checked_object->ActualValue()->IsConstant().

2) Don't delete nodes in place! Instead delay adding checks until they are
necessary:
- if the action is a load, delay the receiver mapcheck in BuildNamedAccess.
- in BuildMonomorphicAccess, add that map-check of the receiver if
info->has_holder(), but don't emit the map-check of the holder.
- in BuildLoadNamedField, emit the map-check of the incoming holder if the
holder is either not an HConstant (hence it's the receiver which is not an
HConstant), or the property is not read-only / non-configurable.

3) Update ComputeReceiverTypes to handle the case where receiver->IsConstant().
That could should either trim down the receiver maps to a transition from the
constant's map; or just add the constant's map as expected receiver type.

Does that sound reasonable?

Regards,
Toon