Loads and stores to global vars are now made via property cell shortcuts installed into parent scri…

Loads and stores to global vars are now made via property cell shortcuts installed into parent script context.

This CL also adds hydrogen stubs for global loads and global stores, full-codegen and TurboFan now uses this machinery.

Committed: https://crrev.com/f87286e2db5b302ecd8331ccd13b1d3859b8bef5
Cr-Commit-Position: refs/heads/master@{#29592}

[Igor Sheludko, 2015-07-07 14:28:28]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2015-07-07 17:40:01]

ishell@chromium.org changed reviewers:
+ verwaest@chromium.org

[Igor Sheludko, 2015-07-07 17:40:02]

PTAL non-TF part (I'll ask TF team to review TF part later)

[Benedikt Meurer, 2015-07-08 08:18:36]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-07-08 08:18:37]

TurboFan part looks good to me.

[Igor Sheludko, 2015-07-08 13:19:16]

The trybots failed because of
https://code.google.com/p/chromium/issues/detail?id=508074

[Benedikt Meurer, 2015-07-08 19:11:51]

LGTM from my side.

[Toon Verwaest, 2015-07-10 16:46:11]

lgtm with some comments. You're idea of adding a flag to be able to turn it off
sounds like a good idea as well.

https://codereview.chromium.org/1224793002/diff/80001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/1224793002/diff/80001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1721:
static_cast<int>(PropertyCellType::kConstantType);
I'd reorder these cases to first handle ConstantType, then Mutable, then
Constant/Uninitialized.

https://codereview.chromium.org/1224793002/diff/80001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/1224793002/diff/80001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:467: if (LookupIterator::DATA == it.state()) {
You should probably do this check before GetProperty since otherwise it'll walk
beyond an interceptor for example. If there's an interceptor, we can essentially
never cache, since it can decide on a whim to make a property appear that wasn't
there before.

https://codereview.chromium.org/1224793002/diff/80001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:472: // This is not a fast case, so keep this
access in a slow mode.
Add comment that you're doing this just to release the property cell containing
the hole, replacing it by the single global empty one.

https://codereview.chromium.org/1224793002/diff/80001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:501: if (LookupIterator::DATA == it.state()) {
Same as above.

[Igor Sheludko, 2015-07-13 08:34:51]

Thanks! Landing...

https://codereview.chromium.org/1224793002/diff/80001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/1224793002/diff/80001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1721:
static_cast<int>(PropertyCellType::kConstantType);
On 2015/07/10 16:46:11, Toon Verwaest wrote:
> I'd reorder these cases to first handle ConstantType, then Mutable, then
> Constant/Uninitialized.

Done, but this stub would probably require further tuning.

https://codereview.chromium.org/1224793002/diff/80001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/1224793002/diff/80001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:467: if (LookupIterator::DATA == it.state()) {
On 2015/07/10 16:46:11, Toon Verwaest wrote:
> You should probably do this check before GetProperty since otherwise it'll
walk
> beyond an interceptor for example. If there's an interceptor, we can
essentially
> never cache, since it can decide on a whim to make a property appear that
wasn't
> there before.

Done.

https://codereview.chromium.org/1224793002/diff/80001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:472: // This is not a fast case, so keep this
access in a slow mode.
On 2015/07/10 16:46:11, Toon Verwaest wrote:
> Add comment that you're doing this just to release the property cell
containing
> the hole, replacing it by the single global empty one.

Done.

https://codereview.chromium.org/1224793002/diff/80001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:501: if (LookupIterator::DATA == it.state()) {
On 2015/07/10 16:46:11, Toon Verwaest wrote:
> Same as above.

Done.

[Igor Sheludko, 2015-07-13 08:34:58]

The CQ bit was checked by ishell@chromium.org

[Igor Sheludko, 2015-07-13 08:34:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1224793002/#ps100001
(title: "Addressing comments")

[commit-bot: I haz the power, 2015-07-13 08:35:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224793002/100001

[commit-bot: I haz the power, 2015-07-13 09:18:51]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-07-13 09:19:11]

Patchset 5 (id:??) landed as
https://crrev.com/f87286e2db5b302ecd8331ccd13b1d3859b8bef5
Cr-Commit-Position: refs/heads/master@{#29592}