Background compile JS extension scripts injected at document start [chromium/src : main]
Luis Pardo (Gerrit)
Luis Pardo voted and added 2 comments![]( "Open in Gerrit")
Votes added by Luis Pardo
| Commit-Queue | +1 |
2 comments
@rdevlin...@chromium.org Can you take a look at this feature? Here is the related [design doc](https://docs.google.com/document/d/1gryv6WZItlP5VbuOwL9VtNnjbM8yvvFpEPnELXWGM9E/edit?usp=sharing). Also linked in the bug.
// TODO(https://crbug.com/436274244): filter out frames that will not have any
// script injections.@rdevlin...@chromium.org Any suggestions on how to address this? It would be nice to only do this work once, so maybe refactoring to fill the pending injections here?
Related details
- Devlin Cronin
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 added 1 comment![]( "Open in Gerrit")
Thanks for the CL! Sorry, I haven't had a chance to get to this yet, but I hope to tomorrow.
One of the concerns I have here is that it seems like this would be difficult to determine if it's a net performance improvement. Moving the compilation to a background thread seems like it has the potential to slow down the compilation of document_start scripts, but since we block the page on the injection of document_start scripts, that would mean we block page load on that.
Is there a rollout plan to ensure we'll measure the impact of this and that we're not regressing any other important metrics?
Related details
- Devlin Cronin
- 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 1 comment![]( "Open in Gerrit")
Thanks for the CL! Sorry, I haven't had a chance to get to this yet, but I hope to tomorrow.
One of the concerns I have here is that it seems like this would be difficult to determine if it's a net performance improvement. Moving the compilation to a background thread seems like it has the potential to slow down the compilation of document_start scripts, but since we block the page on the injection of document_start scripts, that would mean we block page load on that.
Is there a rollout plan to ensure we'll measure the impact of this and that we're not regressing any other important metrics?
I added an [experimentation plan section](https://docs.google.com/document/d/1gryv6WZItlP5VbuOwL9VtNnjbM8yvvFpEPnELXWGM9E/edit?tab=t.0) to the design doc. We'll run an experiment with First Contentful Paint as the success metric, this should catch any regressions on page load, we are also adding other metrics for parameter tunning. I don't think I can run finch trials in Chrome, so I'm setting up the infrastructure in this CL and I'll run the experiment on Edge, it would be great if someone could run the in the Chrome side too.
Related details
- Devlin Cronin
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 1 comment![]( "Open in Gerrit")
- Devlin Cronin
- Sohail Rajdev
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. |
Sohail Rajdev (Gerrit)
Sohail Rajdev added 1 comment![]( "Open in Gerrit")
I see that the design document doesn't have an LGTM from extensions/OWNERS. Please add me again once the design document is signed off from an extensions POV 😊
Related details
- Devlin Cronin
- Luis Pardo
Tim (Gerrit)
Tim added 2 comments![]( "Open in Gerrit")
Doing a bit of a drive-by here saying I think this seems reasonable to experiment with from the extensions side, with it guarded behind the feature flag.
Side note, I think the code coverage check is being thrown off by whatever these ClangTidy build warnings are. I'm going to see if another pass through the bots clears them up.
bool timed_out_;nit: `bool timed_out_ = false;`
This looks like the only real ClangTidy message (although the AttachedFix appears to be backwards somehow). I'm not sure what is up with the other ones though?
Luis Pardo (Gerrit)
Luis Pardo voted and added 2 comments![]( "Open in Gerrit")
Votes added by Luis Pardo
| Commit-Queue | +1 |
Doing a bit of a drive-by here saying I think this seems reasonable to experiment with from the extensions side, with it guarded behind the feature flag.
Side note, I think the code coverage check is being thrown off by whatever these ClangTidy build warnings are. I'm going to see if another pass through the bots clears them up.
Seems like fixing the real warning and rebasing fixed those.
nit: `bool timed_out_ = false;`
This looks like the only real ClangTidy message (although the AttachedFix appears to be backwards somehow). I'm not sure what is up with the other ones though?
- Devlin Cronin
- Tim
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 1 comment![]( "Open in Gerrit")
Devlin Cronin (Gerrit)
Devlin Cronin added 10 comments![]( "Open in Gerrit")
Thanks, Luis! Sorry for the delay here; I thought this was waiting on Tim's review. My mistake.
while (true) {I think base::test::RunUntil() would be a bit cleaner here.
"js": ["script1.js", "script2.js"],Two suggestions:
- Can we use different scripts here, so that if kNonBlockingScript ends up changing, it doesn't break this test? (The NonBlocking aspect just means it doesn't block JS execution; it has nothing to do with script size or compilation)
- Can we verify that these scripts are executed at document_start, so that we guarantee that we don't invalidate assumptions of scripts injected at that point?
BASE_DECLARE_FEATURE(kExtensionsIgnoreBackgroundCompilationForExperiment);I'm not quite sure I understand why we need both of these. Wouldn't we just disable the background compilation feature entirely?
ExtensionsStreamSourceMap;prefer `using` in new code
void StartStreamingJSSources();add a function comment
void StartStreamingJSSources(blink::WebLocalFrame* web_frame,here, too, add a function comment
// TODO(https://crbug.com/436274244): filter out frames that will not have any
// script injections.Devlin Cronin@rdevlin...@chromium.org Any suggestions on how to address this? It would be nice to only do this work once, so maybe refactoring to fill the pending injections here?
Yeah, I'd very much like to avoid doing this matching multiple times -- and this matching is also incomplete.
How much of a cost is there to getting this "wrong"? This might match scripts we don't end up injecting; will that result in significant wasted time?
One of the approaches I'm wondering about is whether we should just store these pending scripts on the ExtensionFrameLoader. It's already the one that's triggering both this call (in ReadyToCommitNavigation) and the call to InjectScripts (when the document element is available). We could fetch the scripts in ReadyToCommit, store them, and then pass those in as scripts to run when injecting the scripts. One concern there is whether the frame could be meaningfully changed between those points in a way that the script would no longer match; for that, I wonder if we should perform a secondary match on the originally-matching scripts to verify they still match (that should be cheaper, since it's a subset of the total scripts).
frame_helper->ClearScriptStreamers();Can the FrameHelper handle this cleanup? It's aware of when we start a new load.
frame_helper->TakeScriptStreamerIfAvailable(script_url);it's interesting to me that these are single-use streamers. Can you expand on what the benefit of that is, compared to a longer-lived streamer-per-URL? (My understanding from skimming the blink code is that they all map to a ref-counted underlying streamer anyway?)
Related details
- Luis Pardo
- Tim
Luis Pardo (Gerrit)
Luis Pardo added 7 comments![]( "Open in Gerrit")
Addressed some feedback, still working through the remaining comments
while (true) {I think base::test::RunUntil() would be a bit cleaner here.
I've never done one of this, please let me know if it is correct. The `base::test::Util` documentation has note about flakiness that I don't fully understand.
BASE_DECLARE_FEATURE(kExtensionsIgnoreBackgroundCompilationForExperiment);I'm not quite sure I understand why we need both of these. Wouldn't we just disable the background compilation feature entirely?
I'm not sure how experimentation works in Chrome. I'd like to compare users that are actively affected by the feature vs those that would have been affected but are part of the control population. I think we need the second flag for this.
prefer `using` in new code
Done
void StartStreamingJSSources();Luis Pardoadd a function comment
Woops, this is an artifact from old code.
void StartStreamingJSSources(blink::WebLocalFrame* web_frame,here, too, add a function comment
Done
frame_helper->TakeScriptStreamerIfAvailable(script_url);it's interesting to me that these are single-use streamers. Can you expand on what the benefit of that is, compared to a longer-lived streamer-per-URL? (My understanding from skimming the blink code is that they all map to a ref-counted underlying streamer anyway?)
There is always one streamer object for each streamed script. In blink, the `InlineScriptStreamer` holds a ref-counted background streamer for thread-safety purposes. In fact, each Inline streamer in blink is [keyed by the full source text of the script](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/script/classic_pending_script.cc;l=56;drc=6fe04ac5cd13eaa7013d9da4b94144b4caa1fabf#:~:text=//%20The%20inline%20script,to%20compile%20here.).
Related details
- Devlin Cronin
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. |