Speedup global_proxy.* attributes/accessors (specialize GlobalProxy access).

Speedup access to global_proxy.* attributes/accessors.

Using a global proxy (e.g. 'window.f', 'w.f' or 'this.f') is considerably slower than evaluating just 'f'.
This CL aims to perform the necessary checks at compile time and inline the accesses.

BUG=chromium:634276

[Alfonso, 2016-09-27 12:49:49]

Description was changed from

==========
Speedup access to window.* attributes/accessors (GlobalProxy access).

Currently loading 'window.f' is considerably slower than evaluating just 'f'.
This CL aims to perform the necessary checks at compile time and make boths
expressions
equivalent performance-wise.

BUG=chromium:634276
==========

to

==========
Speedup access to window.* attributes/accessors (GlobalProxy access).

Currently loading 'window.f' is considerably slower than evaluating just 'f'.
This CL aims to perform the necessary checks at compile time and make boths
expressions equivalent performance-wise.

BUG=chromium:634276
==========

[Alfonso, 2016-09-27 12:49:49]

peterssen@google.com changed reviewers:
+ jochen@chromium.org, vogelheim@chromium.org

[Alfonso, 2016-09-27 12:52:36]

This an early working draft.
PTAL.

[vogelheim, 2016-09-27 13:36:54]

Looks good, generally.

I am concerned about he return NULL thing; someone w/ better understanding of
Crankshaft should review this once the current nits are worked out. (This need
crankshaft/* OWNERs review anyhow.)

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

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:5592: return LoadFromCell(&it, expr->id());
LoadFromCell returns void. You *can* return a void-value, but I find that
confusing.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7697: bool can_access_mono =
info.CanAccessAsMonomorphic(maps);
can_access_monomorphic

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7699: // More than one map? Polymorphic.
This comment comments on one of the sub-clauses in the expression below. I'd
prefer if it would explain the statement intent. Maybe something like this:

// Check for special case: Access via a single map to the global proxy
// can also be handled monomorphically.
bool is_global_proxy_access = !can_access_monomorphic
  && maps->length() == 1
  && maps->first()->IsJSGlobalProxyMap();

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7724: return NULL;
[I don't understand Crankshaft at all, and am merely doing pattern-matching
here. Someone w/ better Crankshaft understanding should review this.]

This *looks* weird. So I take it nullptr is a valid return value; but I'm not
sure what that implies. Maybe LoadFromCell should be changed to return the instr
member. Then the use case in VisitVariableProxy could
ast_context()->ReturnInsttruction(LoadFromCell(..), ..), while this one would
return LoadFromCell(..)

Honestly, not sure what the right thing to do is.

[Alfonso, 2016-09-27 13:38:51]

peterssen@google.com changed reviewers:
+ verwaest@chromium.org

[Alfonso, 2016-09-27 13:39:30]

Toon, PTAL. Is this going on the right direction?

[Toon Verwaest, 2016-09-27 14:57:12]

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

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:5457: bool
HOptimizedGraphBuilder::LookupGlobalPropertyCell(
This should probably be called "CanInlineGlobalPropertyAccess"

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:5494: void
HOptimizedGraphBuilder::LoadFromCell(LookupIterator* it,
And InlineGlobalPropertyAccess

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7700: bool is_global_proxy = maps->length() == 1 &&
!can_access_mono &&
Why doesn't CanAccessMonomorphic just return true for the JSGlobalProxy case? It
*can* access it? ... that's essentailly what you're saying here.

Also, don't forget the compile-time access check. If that fails you won't be
able to inline the access.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7726: }
Doesn't this fall-through mean that you'll try to inline a monomorphic access to
a global proxy for which we couldn't inline the cell path, using a
ProperyAccessInfo that's totally not preparsed to do so?

[Alfonso, 2016-09-29 09:36:40]

PTAL.

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

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:5457: bool
HOptimizedGraphBuilder::LookupGlobalPropertyCell(
On 2016/09/27 14:57:12, Toon Verwaest wrote:
> This should probably be called "CanInlineGlobalPropertyAccess"

Done.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:5494: void
HOptimizedGraphBuilder::LoadFromCell(LookupIterator* it,
On 2016/09/27 14:57:11, Toon Verwaest wrote:
> And InlineGlobalPropertyAccess

Done.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:5592: return LoadFromCell(&it, expr->id());
On 2016/09/27 13:36:54, vogelheim wrote:
> LoadFromCell returns void. You *can* return a void-value, but I find that
> confusing.

Done.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7697: bool can_access_mono =
info.CanAccessAsMonomorphic(maps);
On 2016/09/27 13:36:54, vogelheim wrote:
> can_access_monomorphic

Done.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7699: // More than one map? Polymorphic.
On 2016/09/27 13:36:54, vogelheim wrote:
> This comment comments on one of the sub-clauses in the expression below. I'd
> prefer if it would explain the statement intent. Maybe something like this:
> 
> // Check for special case: Access via a single map to the global proxy
> // can also be handled monomorphically.
> bool is_global_proxy_access = !can_access_monomorphic
>   && maps->length() == 1
>   && maps->first()->IsJSGlobalProxyMap();

Done.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7700: bool is_global_proxy = maps->length() == 1 &&
!can_access_mono &&
CanAccessMonomorphic has many checks and some of them fail with the global
proxy, fixing them to handle the global proxy case is kind of useless since we
don't need the PropertyAccessInfo initialized to load from a cell anyway.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7724: return NULL;
On 2016/09/27 13:36:54, vogelheim wrote:
> [I don't understand Crankshaft at all, and am merely doing pattern-matching
> here. Someone w/ better Crankshaft understanding should review this.]
> 
> This *looks* weird. So I take it nullptr is a valid return value; but I'm not
> sure what that implies. Maybe LoadFromCell should be changed to return the
instr
> member. Then the use case in VisitVariableProxy could
> ast_context()->ReturnInsttruction(LoadFromCell(..), ..), while this one would
> return LoadFromCell(..)
> 
> Honestly, not sure what the right thing to do is.

To my understanding, returning NULL means that the instructions are already
pushed to the stream when the function returns. Returning the instruction allow
further manipulation by the caller.

https://codereview.chromium.org/2369933005/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:7726: }
On 2016/09/27 14:57:11, Toon Verwaest wrote:
> Doesn't this fall-through mean that you'll try to inline a monomorphic access
to
> a global proxy for which we couldn't inline the cell path, using a
> ProperyAccessInfo that's totally not preparsed to do so?

Done.

[Alfonso, 2016-09-29 09:41:11]

Description was changed from

==========
Speedup access to window.* attributes/accessors (GlobalProxy access).

Currently loading 'window.f' is considerably slower than evaluating just 'f'.
This CL aims to perform the necessary checks at compile time and make boths
expressions equivalent performance-wise.

BUG=chromium:634276
==========

to

==========
Speedup access to global_proxy.* attributes/accessors.

Using a global proxy (e.g. 'window.f', 'w.f' or 'this.f') is considerably slower
than evaluating just 'f'.
This CL aims to perform the necessary checks at compile time and inline the
accesses.

BUG=chromium:634276
==========

[Toon Verwaest, 2016-09-29 13:19:47]

Much better! lgtm. vogelheim please review as well :)

[vogelheim, 2016-09-29 15:21:13]

lgtm

https://codereview.chromium.org/2369933005/diff/20001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2369933005/diff/20001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:7705: && maps->first()->IsJSGlobalProxyMap() &&
style nitpick: "  && xxxxx && " is odd. Put the operator either at end or
beginning of the line, but not both.


Ref: https://google.github.io/styleguide/cppguide.html#Boolean_Expressions

https://codereview.chromium.org/2369933005/diff/20001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:7714: }
This is a much nicer place for the logic than what I had previously suggested.
Thanks! :-)

[Alfonso, 2016-09-29 16:17:09]

The CQ bit was checked by peterssen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 16:17:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 16:42:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 16:42:44]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Alfonso, 2016-09-29 19:01:03]

The CQ bit was checked by peterssen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 19:01:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 19:26:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 19:26:23]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Alfonso, 2016-09-30 02:09:23]

The CQ bit was checked by peterssen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 02:09:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 02:38:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 02:38:43]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-09-30 10:49:25]

lgtm with nit

https://codereview.chromium.org/2369933005/diff/80001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2369933005/diff/80001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:7728: return NULL;
nit. nullptr

https://codereview.chromium.org/2369933005/diff/80001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:7750: 
unrelated?

[vogelheim, 2016-11-08 09:30:46]

This work landed (w/ minor changes) as crrev.com/2403003002. Closing this CL.