New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Nov 2
Cc:
Components:
HW: All
OS: All
Priority: 1
Type: Bug

Blocking:
issue 6936



Sign in to add a comment
Lots of %KeyedGetProperty calls because of SeqString/ConsString keys in the babylon test
Project Member Reported by bmeu...@chromium.org, Nov 2 Back to list
In the babylon test of the web-tooling-benchmark we see a lot of calls to %KeyedGetProperty, up 450k adding up to almost 5% of the overall execution. The vast majority of these calls come from the fact that the key is either a SeqString or a ConsString in the KeyedLoadIC. jkummerow@ already introduced machinery earlier to just lookup such keys in the string table and then use the internalized version (hidden behind ---internalize_on_the_fly), but turning on this flag causes a lot of traffic on the megamorphic stub cache.

We could however try to still internalize, but then when we find an entry in the string table do the regular machinery w/o probing the stub cache.
 
Runtime call stats produces something along the lines of this on V8 ToT:

                      Runtime Function/C++ Builtin        Time             Count
========================================================================================
                                      JS_Execution   5267.75ms  77.93%      2259   0.23%
                    GC_SCAVENGER_SCAVENGE_PARALLEL    411.33ms   6.08%       156   0.02%
                                  KeyedGetProperty    313.14ms   4.63%    453391  46.07%
                              RecompileSynchronous    171.58ms   2.54%       447   0.05%
                                    ForInEnumerate    110.92ms   1.64%     24594   2.50%
                              ParseFunctionLiteral     45.78ms   0.68%      2454   0.25%
                                 GrowArrayElements     38.28ms   0.57%        65   0.01%
                    ..............................
wtb-babylon.js
2.9 MB View Download
babylon.json
9.3 MB View Download
Project Member Comment 2 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/6366a010080ec106ad83964060825c0a72c77458

commit 6366a010080ec106ad83964060825c0a72c77458
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Thu Nov 02 10:14:07 2017

[ic] Internalize strings on the fly in KeyedLoadICGeneric.

This turns on the existing --internalize_on_the_fly flag for the
MEGAMORPHIC KeyedLoadIC to properly internalize strings before
looking up the property. This avoids the otherwise taken runtime
call to %KeyedGetProperty, which is definitely slower.

Initially the --internalize_on_the_fly flag was turned off because
internalizing strings on the fly causes too much traffic on the
megamorphic stub cache. We avoid this problem here by not probing
the stub cache in that case, which still gives the benefit of not
having to go to the runtime.

This improves the babylon test on the web-tooling-benchmark by around
2-3% and will probably also help with several tests (like React or
Ember) on the Speedometer benchmark.

If this CL causes trouble (i.e. tanks something important), we can
just turn off the --internalize_on_the_fly flag again.

Bug: v8:6936,  v8:7026 
Change-Id: Ia59a8a3799d9624d831d66b05bae3ecef31cee0a
Reviewed-on: https://chromium-review.googlesource.com/750821
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49072}
[modify] https://crrev.com/6366a010080ec106ad83964060825c0a72c77458/src/flag-definitions.h
[modify] https://crrev.com/6366a010080ec106ad83964060825c0a72c77458/src/ic/accessor-assembler.cc

Status: Fixed
Project Member Comment 4 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e06c116379cc2386d584f6488d673de40248fcc7

commit e06c116379cc2386d584f6488d673de40248fcc7
Author: Michael Achenbach <machenbach@chromium.org>
Date: Thu Nov 02 13:37:35 2017

Revert "[ic] Internalize strings on the fly in KeyedLoadICGeneric."

This reverts commit 6366a010080ec106ad83964060825c0a72c77458.

Reason for revert: Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/19429

Original change's description:
> [ic] Internalize strings on the fly in KeyedLoadICGeneric.
> 
> This turns on the existing --internalize_on_the_fly flag for the
> MEGAMORPHIC KeyedLoadIC to properly internalize strings before
> looking up the property. This avoids the otherwise taken runtime
> call to %KeyedGetProperty, which is definitely slower.
> 
> Initially the --internalize_on_the_fly flag was turned off because
> internalizing strings on the fly causes too much traffic on the
> megamorphic stub cache. We avoid this problem here by not probing
> the stub cache in that case, which still gives the benefit of not
> having to go to the runtime.
> 
> This improves the babylon test on the web-tooling-benchmark by around
> 2-3% and will probably also help with several tests (like React or
> Ember) on the Speedometer benchmark.
> 
> If this CL causes trouble (i.e. tanks something important), we can
> just turn off the --internalize_on_the_fly flag again.
> 
> Bug: v8:6936,  v8:7026 
> Change-Id: Ia59a8a3799d9624d831d66b05bae3ecef31cee0a
> Reviewed-on: https://chromium-review.googlesource.com/750821
> Reviewed-by: Igor Sheludko <ishell@chromium.org>
> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49072}

TBR=ishell@chromium.org,bmeurer@chromium.org

Change-Id: I5345eb29016ecd6b7788b1b49b2f53992ea82b58
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:6936,  v8:7026 
Reviewed-on: https://chromium-review.googlesource.com/750904
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49077}
[modify] https://crrev.com/e06c116379cc2386d584f6488d673de40248fcc7/src/flag-definitions.h
[modify] https://crrev.com/e06c116379cc2386d584f6488d673de40248fcc7/src/ic/accessor-assembler.cc

Project Member Comment 5 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/96b1fdb27689535be5ac44854813711e77dbff41

commit 96b1fdb27689535be5ac44854813711e77dbff41
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Thu Nov 02 20:57:10 2017

[ic] Internalize strings on the fly in KeyedLoadICGeneric.

This turns on the existing --internalize_on_the_fly flag for the
MEGAMORPHIC KeyedLoadIC to properly internalize strings before
looking up the property. This avoids the otherwise taken runtime
call to %KeyedGetProperty, which is definitely slower.

Initially the --internalize_on_the_fly flag was turned off because
internalizing strings on the fly causes too much traffic on the
megamorphic stub cache. We avoid this problem here by not probing
the stub cache in that case, which still gives the benefit of not
having to go to the runtime.

This improves the babylon test on the web-tooling-benchmark by around
2-3% and will probably also help with several tests (like React or
Ember) on the Speedometer benchmark.

If this CL causes trouble (i.e. tanks something important), we can
just turn off the --internalize_on_the_fly flag again.

Bug: v8:6936,  v8:7026 
Change-Id: If295ed3fd013f8b0ff031f9979e7df21dab817b6
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/751464
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49093}
[modify] https://crrev.com/96b1fdb27689535be5ac44854813711e77dbff41/src/flag-definitions.h
[modify] https://crrev.com/96b1fdb27689535be5ac44854813711e77dbff41/src/ic/accessor-assembler.cc
[add] https://crrev.com/96b1fdb27689535be5ac44854813711e77dbff41/test/mjsunit/regress/regress-7026.js

A bit early probably (I suggest going over the top25 manually again):

https://chromeperf.appspot.com/report?sid=417388d393911e8144a9dc6c28a8023cc197884e4a004c6b4c6c578f1b0365d7

- Runtime -19ms
- Javascript: +20.5ms
- IC: + 5.5ms


For most other pages this doesn't seem to move the needle much for now.
Sign in to add a comment