New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 769203 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-11-03
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 783124



Sign in to add a comment

Add more detailed UMA for V8 code caching

Project Member Reported by kinuko@chromium.org, Sep 27 2017

Issue description

From our discussion in Speed Convergence.

We're currently using some heuristics (3 visits in 72 hours, for >1k script) for code caching but hit rate is not very high (20%-ish). It'd be desirable to have more detailed metrics/UMA around that to understand when/how we're hitting or missing the cache so that we could potentially improve it.

(Assigning this to hiroshige@ with kouhei@ per what's discussed, but feel free to reassign as needed)
 

Comment 1 by kinuko@chromium.org, Sep 27 2017

Cc: -mcilroy@chromium.org rmcilroy@chromium.org
Cc: leszeks@chromium.org mythria@chromium.org
Any update from this? We'd like to start thinking about reconsidering these heuristics, so it would be great to know which are hurting the hit-rate the most.

Comment 3 by kouhei@chromium.org, Oct 16 2017

Cc: hirosh...@chromium.org
NextAction: 2017-10-17
Owner: kouhei@chromium.org
I'll try to find time today or tomorrow.

Comment 4 by kouhei@chromium.org, Oct 16 2017

Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/933a8ea23eed36537593990fe4c5f77a17c9834d

commit 933a8ea23eed36537593990fe4c5f77a17c9834d
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Mon Oct 16 15:54:43 2017

V8ScriptRunner: Remove CompileAndConsumeOrProduce

The method was introduced as a helper method back in the days, but given
that now parser-cache is the only codepath which use this, and the selection
logic is centralized in SelectCompileFunction, the code is easier to read
when inlined.

Bug:  769203 
Change-Id: I64d7871d47e087de95fc60c04726ffcc486d1741
Reviewed-on: https://chromium-review.googlesource.com/720538
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509062}
[modify] https://crrev.com/933a8ea23eed36537593990fe4c5f77a17c9834d/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp

The NextAction date has arrived: 2017-10-17

Comment 8 by kouhei@chromium.org, Oct 17 2017

NextAction: 2017-10-21
CL still in review
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32947e78ae167c2a12c313cd8666bf90b64a3618

commit 32947e78ae167c2a12c313cd8666bf90b64a3618
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Tue Oct 17 21:29:00 2017

V8ScriptRunner: Add "V8.CompileHeuristicsDecision" UMA

This CL introduces "V8.CompileHeuristicsDecision" which records
which of the script compilation function got picked, providing more insights
about how often V8 {code,parser} cache is {produced,consumed,bypassed}.

Bug:  769203 
Change-Id: I7b27a1634ee5b1623330acae1ef27bc78f39742e
Reviewed-on: https://chromium-review.googlesource.com/720660
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509540}
[modify] https://crrev.com/32947e78ae167c2a12c313cd8666bf90b64a3618/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
[modify] https://crrev.com/32947e78ae167c2a12c313cd8666bf90b64a3618/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/32947e78ae167c2a12c313cd8666bf90b64a3618/tools/metrics/histograms/histograms.xml

NextAction: 2017-11-03
Thanks kouhei@. Let's set about 2+ weeks from now for next action to review the numbers.
I was looking at the UMA numbers for the code caching decisions, and ~87% of the time we don't cache the code because of the reason "NoCacheHandler". In which situations, do we hit this case?

Also another interesting observation is the % of ConsumeCodeCache is (~0.69%) is much smaller than the % of ProduceCodeCache (~4.68%).

Data is here:
https://uma.googleplex.com/p/chrome/histograms/?endDate=20171028&dayCount=7&histograms=V8.CompileHeuristicsDecision&fixupData=true&showMax=true&filters=isofficial%2Ceq%2CTrue&implicitFilters=isofficial
There is a document here:
https://docs.google.com/document/d/1-_LIjzV5-wnNXoOvPa4DlPvm-_EwUJtjOfMUsImExtk/edit

TL;DR: We don't have a cache handler if the script is not backed by a HttpResource, e.g. inline scripts or evals.
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce

commit f571da954cbb0f8881f4d9da6a8dcdf219ee4dce
Author: Leszek Swirski <leszeks@chromium.org>
Date: Wed Nov 01 17:10:45 2017

[compiler] Split compilation timer on caching decision

Rather than having a single script compilation timer, split it into
multiple timers depending on the state of the (blink-owned) code cache
and (v8-owned) complation cache. This is intended to replace both the
script compilation time timer, and the compilation heuristic enum.

Also keep track of why blink might not want us to produce (or consume) a
code cache, and split the compilation timer on this as well.

Note, there is currently no timer for streaming sources, so these won't
show up in the histograms.

Bug: chromium:582873
Bug:  chromium:769203 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: Ia32fff044f919e20e3cec73329e62e01e421b72a
Reviewed-on: https://chromium-review.googlesource.com/746922
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49065}
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/include/v8.h
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/src/api.cc
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/src/bootstrapper.cc
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/src/compiler.cc
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/src/compiler.h
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/src/counters.h
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/test/cctest/compiler/test-linkage.cc
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/test/cctest/test-compiler.cc
[modify] https://crrev.com/f571da954cbb0f8881f4d9da6a8dcdf219ee4dce/test/cctest/test-serialize.cc

The NextAction date has arrived: 2017-11-03
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37363e67e0a89d927a8fbc3f6df3fd601cf1bd9e

commit 37363e67e0a89d927a8fbc3f6df3fd601cf1bd9e
Author: Leszek Swirski <leszeks@chromium.org>
Date: Fri Nov 03 12:58:24 2017

[v8] Pass cache decisions through to V8 for histograms

V8's API allows us to pass code cache decisions, and keeps track of
compilation time for each of these decisions. So, instead of tracking
these decisions on the Chromium side, pass the cache/no cache decisions
through to V8.

See also https://chromium-review.googlesource.com/746922

Bug:  chromium:769203 
Change-Id: Idba176050fb7c4d7b9a81e6e4a14878d8972c62c
Reviewed-on: https://chromium-review.googlesource.com/746804
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513755}
[modify] https://crrev.com/37363e67e0a89d927a8fbc3f6df3fd601cf1bd9e/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
[modify] https://crrev.com/37363e67e0a89d927a8fbc3f6df3fd601cf1bd9e/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/37363e67e0a89d927a8fbc3f6df3fd601cf1bd9e/tools/metrics/histograms/histograms.xml

In the earlier UMA data (before Leszek's changes), "ConsumeCodeCache" actually means "ProduceCodeCache" and vice versa. There was a mistake in matching the enum value to the name of histogram. enum value of 6 corresponds to "ConsumeCodeCache" but the name corresponding to it was "ProduceCodeCache". Considering this, from the UMA data we can see we consume much more than what we produce which is a good news. So working towards increasing the cases where we can cache the code and/or increasing the amount we cache should help.
Huh. That definitely sounds like a good explanation. Thanks for the investigation!
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9dd2950095da4af11453b87d8d4b62e2567413d7

commit 9dd2950095da4af11453b87d8d4b62e2567413d7
Author: Leszek Swirski <leszeks@chromium.org>
Date: Mon Nov 06 19:22:18 2017

[v8] Add "because extension" to "cache behaviour" enum

Adds the enums.xml entry corresponding to the new enum value added in
https://chromium-review.googlesource.com/753740

Bug:  chromium:769203 
Change-Id: I1b36160a47fee052d1e8710c912dbadcde0d3304
Reviewed-on: https://chromium-review.googlesource.com/754933
Reviewed-by: Mythri Alle <mythria@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514206}
[modify] https://crrev.com/9dd2950095da4af11453b87d8d4b62e2567413d7/tools/metrics/histograms/enums.xml

Project Member

Comment 19 by bugdroid1@chromium.org, Nov 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/5b0a753d2aa8498c6f44d187dd6fe64c18c02efc

commit 5b0a753d2aa8498c6f44d187dd6fe64c18c02efc
Author: Leszek Swirski <leszeks@chromium.org>
Date: Mon Nov 06 20:07:38 2017

[code-cache] Keep track of extensions not caching

Bug:  chromium:769203 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: Iffb7e92fb9c08c42f03ad28c8defb516454a2d3d
Reviewed-on: https://chromium-review.googlesource.com/753740
Reviewed-by: Mythri Alle <mythria@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49158}
[modify] https://crrev.com/5b0a753d2aa8498c6f44d187dd6fe64c18c02efc/include/v8.h
[modify] https://crrev.com/5b0a753d2aa8498c6f44d187dd6fe64c18c02efc/src/bootstrapper.cc
[modify] https://crrev.com/5b0a753d2aa8498c6f44d187dd6fe64c18c02efc/src/compiler.cc
[modify] https://crrev.com/5b0a753d2aa8498c6f44d187dd6fe64c18c02efc/src/counters.h

Project Member

Comment 20 by bugdroid1@chromium.org, Nov 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b12bfdca61a4076924d8c3e4e3002b990344d231

commit b12bfdca61a4076924d8c3e4e3002b990344d231
Author: Leszek Swirski <leszeks@chromium.org>
Date: Tue Nov 07 10:38:31 2017

[v8] Fix histogram name to match V8

The V8 compile time IsolateCacheHit histogram is incorrectly named
CompilationCacheHit, a name we used in initial patchsets but not the one
we decided to use in the end. There is no data under the incorrect name.

Bug:  chromium:769203 
Change-Id: I05e3173a160722716094a31c4184a41440938f5a
Reviewed-on: https://chromium-review.googlesource.com/754605
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514438}
[modify] https://crrev.com/b12bfdca61a4076924d8c3e4e3002b990344d231/tools/metrics/histograms/histograms.xml

Blocking: 783124
So at a first look, it looks like we're getting a whole lot of kNoCacheNoReason	and kNoCacheBecauseNoResource (where NoResource is some reason other than inline script).

kNoCacheNoReason is the default enum value, used by anything that calls compile without passing through another no-cache reason. In practice, this means script compiles coming from somewhere other than V8ScriptRunner. I see Script(Compiler)?::Compile in the following files:

  1. gin/shell_runner.cc
  2. net/proxy/proxy_resolver_v8.cc
  3. extensions/renderer/script_context.cc

2 is (afaik) PAC files, and 3 is (I believe?) extensions, which don't exist on Android where the kNoCacheNoReason count is still high -- so, I'm guessing the culprit is 1. I'm not 100% sure what the "shell runner" is, I'd appreciate input here.

kNoCacheBecauseNoResource is special cased to kNoCacheBecauseInlineScript when script offsets are non-zero, but otherwise any empty cache handler is reported as "no resource". I'm not sure if event handlers, set timeout, eval, that sort of thing go through this path and are reported as no resource, I'd appreciate some input from bindings on this also.

I would like to split up these two buckets into sub-buckets. I can wire through additional no-cache reasons for things calling Script::Compile, but I'm not sure how to distinguish reasons why a resource might not exist. Again, I would appreciate help from someone with better knowledge of the bindings. We should also split up these bins soon.
I thought shell_runner is just a test executable and isn't part of chromium - I might be wrong though.
Clicking around in code search, that does indeed seem to be the case. So, I'm a bit stumped, every other use seems to be tests :/
Can you add some debug breakpoints and manually surf the web to see where this might be coming from?
Good idea, I'll have a go at that tomorrow.
Project Member

Comment 27 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/693d7adb9fef2613e6745f6234e71c9685349d22

commit 693d7adb9fef2613e6745f6234e71c9685349d22
Author: Leszek Swirski <leszeks@chromium.org>
Date: Fri Nov 10 13:31:12 2017

[compiler] Log isolate cache hits conflicting with code cache

Keep separate track of isolate cache hits that conflict with
producing/consuming the code cache, so that we can see how many code
cache hits are "stolen" by the isolate cache, and how many isolate cache
entries are "wasted" by recompiling for cache production.

Bug:  chromium:769203 
Change-Id: I3d8dbfc6a8981b779eb073176454ad43dfbcbaaf
Reviewed-on: https://chromium-review.googlesource.com/763368
Reviewed-by: Mythri Alle <mythria@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49293}
[modify] https://crrev.com/693d7adb9fef2613e6745f6234e71c9685349d22/src/compiler.cc
[modify] https://crrev.com/693d7adb9fef2613e6745f6234e71c9685349d22/src/counters.h

Project Member

Comment 28 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b310982411a055d7e253b32b7b62d415329c262

commit 6b310982411a055d7e253b32b7b62d415329c262
Author: Leszek Swirski <leszeks@chromium.org>
Date: Fri Nov 10 16:46:47 2017

[v8] Add isolate cache hit variant enum values

Adds the enums.xml entries corresponding to the new enum values added
in https://chromium-review.googlesource.com/763368

Bug:  chromium:769203 
Change-Id: I8815297687b63a9681bc396dab3eadc112e9aad1
Reviewed-on: https://chromium-review.googlesource.com/763508
Reviewed-by: Mythri Alle <mythria@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515573}
[modify] https://crrev.com/6b310982411a055d7e253b32b7b62d415329c262/tools/metrics/histograms/enums.xml

Project Member

Comment 29 by bugdroid1@chromium.org, Nov 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/13d0784dc1e041f533e997bd853fb7905c23f740

commit 13d0784dc1e041f533e997bd853fb7905c23f740
Author: Leszek Swirski <leszeks@chromium.org>
Date: Mon Nov 13 12:11:03 2017

[compiler] Fix isolate cache/consume cache histogram sample

The consume code cache code path was only taken in the case where the
isolate cache lookup failed, making the "hit isolate cache when consume
code cache" histogram sample never work.

Bug:  chromium:769203 
Change-Id: I15398f9ce4fc53602b323b8efb8ac9787440dd85
Reviewed-on: https://chromium-review.googlesource.com/765455
Reviewed-by: Mythri Alle <mythria@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49321}
[modify] https://crrev.com/13d0784dc1e041f533e997bd853fb7905c23f740/src/compiler.cc

So, manual surfing on Desktop only surfaced extensions/renderer/script_context.cc, via extensions/renderer/module_system.cc. On Android, this code isn't even compiled, so that can't be it. Next is heavier printf debugging on Android I guess.
Project Member

Comment 31 by bugdroid1@chromium.org, Nov 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/61e04e2867d5057ae594316112ad48d4e9407d4f

commit 61e04e2867d5057ae594316112ad48d4e9407d4f
Author: Leszek Swirski <leszeks@chromium.org>
Date: Tue Nov 14 19:46:45 2017

[compiler] Add new "no cache reason" enum values

Add enum values to the "no cache reason" API which reflect new types of
no-cache reason we will want to distinguish.

Also, renames one of the enum values (BecauseExtension ->
BecauseV8Extension) because it was confusing. It's a V8-only type of no
cache reason, so it shouldn't affect embedders.

Bug:  chromium:769203 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I41d4ecfb35b2e91b71562b4f23b15d20f16a943c
Reviewed-on: https://chromium-review.googlesource.com/769010
Reviewed-by: Mythri Alle <mythria@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49367}
[modify] https://crrev.com/61e04e2867d5057ae594316112ad48d4e9407d4f/include/v8.h
[modify] https://crrev.com/61e04e2867d5057ae594316112ad48d4e9407d4f/src/bootstrapper.cc
[modify] https://crrev.com/61e04e2867d5057ae594316112ad48d4e9407d4f/src/compiler.cc
[modify] https://crrev.com/61e04e2867d5057ae594316112ad48d4e9407d4f/src/counters.h

Owner: leszeks@chromium.org
Reassigning to Leszek who is looking at this now.
Project Member

Comment 33 by bugdroid1@chromium.org, Nov 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d148e138e19ed0c3f43fb4f714ec8a0338213392

commit d148e138e19ed0c3f43fb4f714ec8a0338213392
Author: Leszek Swirski <leszeks@chromium.org>
Date: Wed Nov 15 16:50:35 2017

[v8] Wire through extension module "no code cache" reason

Skip the Script::Compile convenience method, and use
ScriptCompiler::Compile directly, to wire through a "no cache reason"
for extension modules, allowing us to seperate them from the
surprisingly large "no reason" bucket in the UMA.

Also, update enums.xml for the new enum values introduced in v8, see
https://chromium-review.googlesource.com/769010

Bug:  chromium:769203 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I3ea79bb376e43c657b57e873f4954c74aa05b651
Reviewed-on: https://chromium-review.googlesource.com/769027
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516721}
[modify] https://crrev.com/d148e138e19ed0c3f43fb4f714ec8a0338213392/extensions/renderer/module_system.cc
[modify] https://crrev.com/d148e138e19ed0c3f43fb4f714ec8a0338213392/extensions/renderer/script_context.cc
[modify] https://crrev.com/d148e138e19ed0c3f43fb4f714ec8a0338213392/extensions/renderer/script_context.h
[modify] https://crrev.com/d148e138e19ed0c3f43fb4f714ec8a0338213392/tools/metrics/histograms/enums.xml

Project Member

Comment 34 by bugdroid1@chromium.org, Nov 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/441dee979ff9a4847c2e20f684d18631efc84c46

commit 441dee979ff9a4847c2e20f684d18631efc84c46
Author: Leszek Swirski <leszeks@chromium.org>
Date: Wed Nov 15 17:02:41 2017

[v8] Wire through PAC script "no code cache" reason

Skip the Script::Compile convenience method, and use
ScriptCompiler::Compile directly, to wire through a "no cache reason"
for PAC files, allowing us to seperate them from the surprisingly large
"no reason" bucket in the UMA.

Bug:  chromium:769203 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id808878a81b3b843ecd103a533805fda7be194a9
Reviewed-on: https://chromium-review.googlesource.com/771212
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516723}
[modify] https://crrev.com/441dee979ff9a4847c2e20f684d18631efc84c46/net/proxy/proxy_resolver_v8.cc

Project Member

Comment 35 by bugdroid1@chromium.org, Nov 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53bcd586175cdade5b0f7693335419e8789d09b7

commit 53bcd586175cdade5b0f7693335419e8789d09b7
Author: Leszek Swirski <leszeks@chromium.org>
Date: Fri Nov 17 17:15:48 2017

[v8] Log the source origin of scripts passed to V8

For V8 logs and UMA, we want to detect whether scripts passed to V8 are
from an external file, inline scripts, generated strings or other
sources. Previously we were doing this by checking source offsets and
the presence of a resource cache handler, but this was inaccurate, not
to mention a layer violation.

This introduces a ScriptSourceOrigin enum which is made a member of
ScriptSourceCode. The ScriptLoader tells the ClassicPendingScript
whether its source is coming from an inline script, is generated during
a document.write, or more esoteric variants like is from a non-parser
generated element.

Then, the V8ScriptRunner can use this enum to decide what sort of "no
cache handler" reason it should pass to V8, which is logged in UMA.

I have also opportunistically updated other users of ScriptSourceCode
that don't necessarily use script tags. We may want to track these
individually in the future.

Bug:  chromium:769203 
Change-Id: I24f1e7a5b235246e7d4e2b4753b8aec112239832
Reviewed-on: https://chromium-review.googlesource.com/771793
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517439}
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/bindings.gni
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/ScriptController.h
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h
[add] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/ScriptSourceLocationType.h
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunnerTest.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/dom/ClassicPendingScript.h
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/exported/WebPluginContainerImpl.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/exported/WebScriptSource.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/inspector/InspectorOverlayAgent.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
[modify] https://crrev.com/53bcd586175cdade5b0f7693335419e8789d09b7/third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp

Project Member

Comment 36 by bugdroid1@chromium.org, Nov 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/25820bdab9c1092dcde6dbfdfeaeafcaba188e67

commit 25820bdab9c1092dcde6dbfdfeaeafcaba188e67
Author: Leszek Swirski <leszeks@chromium.org>
Date: Fri Nov 17 18:16:44 2017

[code-cache] Log resources with no cache handler

Add another entry to the NoCacheReason enum, reporting that the chromium
ScriptResource has no cache handler.

Also, the amount of chromium-specific entries in this enum is getting
too high. So, added a TODO for removing them -- possibly in the future
we want to do this no-cache reason logging in Chromium after all,
propagating isolate cache hits and consume failures back up the API with
an out parameter.

Bug:  chromium:769203 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I63ca863cfef61e04e7104318eb79810796b61a9c
Reviewed-on: https://chromium-review.googlesource.com/776893
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49458}
[modify] https://crrev.com/25820bdab9c1092dcde6dbfdfeaeafcaba188e67/include/v8.h
[modify] https://crrev.com/25820bdab9c1092dcde6dbfdfeaeafcaba188e67/src/compiler.cc
[modify] https://crrev.com/25820bdab9c1092dcde6dbfdfeaeafcaba188e67/src/counters.h

Project Member

Comment 37 by bugdroid1@chromium.org, Nov 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3654e25f47d9baf1818e021d2fd8baf8c992312

commit a3654e25f47d9baf1818e021d2fd8baf8c992312
Author: Leszek Swirski <leszeks@chromium.org>
Date: Tue Nov 21 19:17:30 2017

[v8] Log ScriptResources with no cache handler

Seperate the logs for missing cache handlers when there is no
ScriptResource, and ScriptResources with null cache handlers.

Bug:  chromium:769203 
Change-Id: I7abc1f5330ca2666b9ed67ddf1b54947598c23ca
Reviewed-on: https://chromium-review.googlesource.com/778544
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518327}
[modify] https://crrev.com/a3654e25f47d9baf1818e021d2fd8baf8c992312/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp

Project Member

Comment 38 by bugdroid1@chromium.org, Nov 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/927aaa648694e580253423a018f21dc35de8b1bb

commit 927aaa648694e580253423a018f21dc35de8b1bb
Author: Leszek Swirski <leszeks@chromium.org>
Date: Fri Nov 24 12:31:03 2017

[v8] Add missing no-cache enum value

Add the "script resource with no cache handler" no-cache reason to
enums.xml

Bug:  chromium:769203 
Change-Id: Id5a6c8052b3cb933da67db4c1dd6fabf5b809383
Reviewed-on: https://chromium-review.googlesource.com/787900
Reviewed-by: Mythri Alle <mythria@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519091}
[modify] https://crrev.com/927aaa648694e580253423a018f21dc35de8b1bb/tools/metrics/histograms/enums.xml

Status: Fixed (was: Started)
Good enough now, I think.

Sign in to add a comment