Background compile JS extension scripts injected at document start [chromium/src : main]
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
base::UmaHistogramBoolean("WebCore.Scripts.InlineStreamerTimedOut",
timed_out_);Luis Pardosimilarly, would be nice to handle UMA histograms consistently inside or outside this method
Leszek SwirskiDo you mean emitting one histogram for a real inline scripts and another for extension scripts? The `Source` caller doesn't know which kind of script it is compiling and we don't need it for extensions scripts since we already record the compilation time for scripts that timedout and scripts that didn't. I added the histogram name as a class field and emitting the histogram only when the name is explicitly set.
we don't need it for extensions scripts since we already record the compilation time for scripts that timedout and scripts that didn't.
This was exactly what I meant, it's an awkward inconsistency that extension scripts collect time-out histograms from the outside, while non-extension scripts collect it in here. If you're exposing timeout state, then probably it's best to collect this from the outside for non-extension inline scripts too, with a similar method to your EmitCompilationHistograms here.
Related details
- Devlin Cronin
- Jeremy Roman
- Justin Lulejian
- Luis Pardo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 2 comments![]( "Open in Gerrit")
@jbr...@chromium.org @les...@chromium.org @rdevlin...@chromium.org Thanks for the reviews. I'll be unresponsive starting tomorrow, I'll leave this in auto-submit and hopefully it can be merged as is. @emro...@microsoft.com will be taking over this work and he could address any remaining feedback in a follow-up CL.
base::UmaHistogramBoolean("WebCore.Scripts.InlineStreamerTimedOut",
timed_out_);Luis Pardosimilarly, would be nice to handle UMA histograms consistently inside or outside this method
Leszek SwirskiDo you mean emitting one histogram for a real inline scripts and another for extension scripts? The `Source` caller doesn't know which kind of script it is compiling and we don't need it for extensions scripts since we already record the compilation time for scripts that timedout and scripts that didn't. I added the histogram name as a class field and emitting the histogram only when the name is explicitly set.
we don't need it for extensions scripts since we already record the compilation time for scripts that timedout and scripts that didn't.
This was exactly what I meant, it's an awkward inconsistency that extension scripts collect time-out histograms from the outside, while non-extension scripts collect it in here. If you're exposing timeout state, then probably it's best to collect this from the outside for non-extension inline scripts too, with a similar method to your EmitCompilationHistograms here.
Added a TODO for now. @emro...@microsoft.com can follow-up in another CL.
Related details
- Devlin Cronin
- Emmanuel Romero Ruiz
- Jeremy Roman
- Justin Lulejian
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 4 comments![]( "Open in Gerrit")
Thanks Luis! Had a couple comments.
compilation in the background thread earlier in the compilation process,
this CL, makes changes to extensions/ and blink/ to make this happen:
```suggestion
compilation in the background thread earlier in the compilation process. This CL makes changes to extensions/ and blink/ to make this happen:
```
// Executed on the main thread onlyhttps://google.github.io/styleguide/cppguide.html#Punctuation_Spelling_and_Grammar
```suggestion
// Executed on the main thread only.
```
Number of scripts injected at document start that were eligible for
background compilation.Let's document precisely when a histogram is emitted, in addition to what is emitted[[1]](https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md#State-When-It-Is-Recorded). So let's do that for each new histogram. For example:
```suggestion
Number of scripts to be injected at document start that were eligible for
background compilation.
Emitted when `extensions_features::kExtensionsBackgroundCompilation` is
enabled, the navigation is ready to commit, and the scripts have started
streaming to v8 for compilation.
```
If the histograms are so closely related that they'd probably be added/removed together (example: `...EligibleScriptsPercentage`) I think it's fine to say:
`Emitted at the same time as Extensions.BackgroundCompileInjectedScripts.EligibleScriptsCount`.
Related details
- Devlin Cronin
- Emmanuel Romero Ruiz
- Jeremy Roman
- Leszek Swirski
- Luis Pardo
Luis Pardo (Gerrit)
Luis Pardo voted and added 3 comments![]( "Open in Gerrit")
Votes added by Luis Pardo
| Auto-Submit | +1 |
3 comments
compilation in the background thread earlier in the compilation process,
this CL, makes changes to extensions/ and blink/ to make this happen:
```suggestion
compilation in the background thread earlier in the compilation process. This CL makes changes to extensions/ and blink/ to make this happen:```
Done
https://google.github.io/styleguide/cppguide.html#Punctuation_Spelling_and_Grammar
```suggestion
// Executed on the main thread only.
```
Done
Number of scripts injected at document start that were eligible for
background compilation.Let's document precisely when a histogram is emitted, in addition to what is emitted[[1]](https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md#State-When-It-Is-Recorded). So let's do that for each new histogram. For example:
```suggestion
Number of scripts to be injected at document start that were eligible for
background compilation.Emitted when `extensions_features::kExtensionsBackgroundCompilation` is
enabled, the navigation is ready to commit, and the scripts have started
streaming to v8 for compilation.
```If the histograms are so closely related that they'd probably be added/removed together (example: `...EligibleScriptsPercentage`) I think it's fine to say:
`Emitted at the same time as Extensions.BackgroundCompileInjectedScripts.EligibleScriptsCount`.
- Devlin Cronin
- Emmanuel Romero Ruiz
- Jeremy Roman
- Justin Lulejian
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
1 comment
Related details
- Emmanuel Romero Ruiz
- Jeremy Roman
- Justin Lulejian
- Leszek Swirski
- Luis Pardo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
- Emmanuel Romero Ruiz
- Jeremy Roman
- Justin Lulejian
- Luis Pardo