Add feature flag for inline script cache [chromium/src : main]
Takashi Nakayama (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
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
: v8::ScriptCompiler::kNoCacheBecauseInlineScript;we can keep this, it's distinct from `kNoCacheBecauseCacheTooCold` which is set below if there is a cache handler but no cache inside. It could be worthwhile to extend the CacheBehaviour enum to include a separate `kNoCacheBecauseInlineScriptCacheTooCold`
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Takashi Nakayama
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. |
Kouhei Ueno (Gerrit)
Kouhei Ueno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kouhei Ueno
| Code-Review | +1 |
1 comment
Related details
- Hiroshige Hayashizaki
- Takashi Nakayama
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. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hiroshige Hayashizaki
| Code-Review | +1 |
2 comments
if (source_text_length < features::kInlineScriptCacheMinScriptLength.Get()) {Add `source_location_type == ScriptSourceLocationType::kInline &&`
(Otherwise, `kInlineScriptCacheMinScriptLength` would be applied to non-inline scripts if `kInlineScriptCacheMinScriptLength < kMinimalCodeLengthForNonInlineScript`.
no_cache_reason = v8::ScriptCompiler::kNoCacheBecauseScriptTooSmall;nit: `CHECK(features::IsInlineScriptCacheEnabled());`
Related details
- Takashi Nakayama
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 3 comments![]( "Open in Gerrit")
: v8::ScriptCompiler::kNoCacheBecauseInlineScript;we can keep this, it's distinct from `kNoCacheBecauseCacheTooCold` which is set below if there is a cache handler but no cache inside. It could be worthwhile to extend the CacheBehaviour enum to include a separate `kNoCacheBecauseInlineScriptCacheTooCold`
Thanks for suggestion! I reverted this change and added `v8::ScriptCompiler::kNoCacheBecauseInlineScriptCacheTooCold`. PTAL
if (source_text_length < features::kInlineScriptCacheMinScriptLength.Get()) {Add `source_location_type == ScriptSourceLocationType::kInline &&`
(Otherwise, `kInlineScriptCacheMinScriptLength` would be applied to non-inline scripts if `kInlineScriptCacheMinScriptLength < kMinimalCodeLengthForNonInlineScript`.
Done
no_cache_reason = v8::ScriptCompiler::kNoCacheBecauseScriptTooSmall;Takashi Nakayamanit: `CHECK(features::IsInlineScriptCacheEnabled());`
Added `features::IsInlineScriptCacheEnabled() &&` to the if condition instead.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- 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. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hiroshige Hayashizaki
| Code-Review | +1 |
1 comment
source_text_length < features::kInlineScriptCacheMinScriptLength.Get()) {still I prefer `CHECK(features::IsInlineScriptCacheEnabled());` here, as it's clearer that Lines 434-449 is just applying different threshold for inline/external scripts.
The current patch set, the `features::IsInlineScriptCacheEnabled() &&` inside `if` makes me wonder:
Hmm, then if `features::IsInlineScriptCacheEnabled()` is false, no length limit is imposed for `kInline`?
Actually this is unnecessary thought, as the cache handler should be null and we should have early-returned in such cases. I prefer `CHECK()`ing instead, to avoid such unnecessary thoughts.
Related details
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 1 comment![]( "Open in Gerrit")
source_text_length < features::kInlineScriptCacheMinScriptLength.Get()) {still I prefer `CHECK(features::IsInlineScriptCacheEnabled());` here, as it's clearer that Lines 434-449 is just applying different threshold for inline/external scripts.
The current patch set, the `features::IsInlineScriptCacheEnabled() &&` inside `if` makes me wonder:
Hmm, then if `features::IsInlineScriptCacheEnabled()` is false, no length limit is imposed for `kInline`?
Actually this is unnecessary thought, as the cache handler should be null and we should have early-returned in such cases. I prefer `CHECK()`ing instead, to avoid such unnecessary thoughts.
Done
Related details
- Kouhei Ueno
- 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. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
: v8::ScriptCompiler::kNoCacheBecauseInlineScript;Takashi Nakayamawe can keep this, it's distinct from `kNoCacheBecauseCacheTooCold` which is set below if there is a cache handler but no cache inside. It could be worthwhile to extend the CacheBehaviour enum to include a separate `kNoCacheBecauseInlineScriptCacheTooCold`
Thanks for suggestion! I reverted this change and added `v8::ScriptCompiler::kNoCacheBecauseInlineScriptCacheTooCold`. PTAL
- Kouhei Ueno
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
21 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Add feature flag for inline script cache
Introduce the `kInlineScriptCache` feature flag and associated
parameters to support caching for inline scripts. Since
InlineScriptCache is built upon PersistentCodeCache, a new helper
function `IsInlineScriptCacheEnabled()` is introduced to ensure the
feature activates when all necessary flags are set.
This CL also introduces a configurable minimum script length check for
inline scripts.
DesignDoc:
https://docs.google.com/document/d/1pVFb79e5vkKJI7nZ15BXZFbNji_mB2Y8eZPGhEpK3jE/edit?usp=sharing
- M third_party/blink/common/features.cc
- M third_party/blink/public/common/features.h
- M third_party/blink/renderer/bindings/core/v8/v8_code_cache.cc
Code-Review: +1 by Hiroshige Hayashizaki, +1 by Leszek Swirski
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |