Prefetch img, scripts and stylesheets during parsing #24148

asajeffrey · 2019-09-05T17:20:11Z

Eagerly tokenize html input, and scan for script and img tags which can be prefetched into the cache. This allows resources to be loaded concurrently, which would otherwise be loaded sequentially due to being blocked by script execution. On the cloth webgl demo, this takes the time-to-paint down from 732ms to 560ms.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #3847
These changes do not require tests because it's a perf improvement

This change is

highfive · 2019-09-05T17:20:16Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/servoparser/mod.rs, components/script/dom/servoparser/prefetch.rs, components/net/resource_thread.rs, components/net_traits/lib.rs

highfive · 2019-09-05T17:20:18Z

Warning

These commits modify net and script code, but no tests are modified. Please consider adding a test!

asajeffrey · 2019-09-05T17:25:22Z

IRC chat: https://mozilla.logbot.info/servo/20190904#c16590059 and https://mozilla.logbot.info/servo/20190905#c16593012

asajeffrey · 2019-09-06T13:43:10Z

r? @jdm

asajeffrey · 2019-09-06T13:46:16Z

One thing I'm not sure about is that we're issuing a GET when prefetching, even for content which isn't cacheable. We should probably first issue a HEAD to see if the content is cacheable. Perhaps this should be a follow-up issue?

jdm

I like the simplicity of this model. We just need to make sure that we don't unintentionally bypass security features of the platform by prefetching more liberally than the actual request the page intends to make.

components/script/dom/servoparser/prefetch.rs

jdm · 2019-09-06T13:56:12Z

components/script/dom/servoparser/prefetch.rs

+    inner: HtmlTokenizer<PrefetchSink>,
+}
+
+unsafe_no_jsmanaged_fields!(Tokenizer);


I would prefer to derive JSTraceable on Tokenizer instead to avoid accidents in the future if we update that struct.

Oh this is weird, I replied to this comment but github appears to have lost the reply...

Anyway, the problem is that HtmlTokenizer doesn't implement JSTraceable, and it's difficult to do so as its generic, and the tokenizer API provides no way to access the sink. If it did, the implementation would be pretty easy:

impl<Sink: JSTraceable> for HtmlTokenizer<Sink> { fn trace(&self) { self.sink().trace() } }

but there's no sink() method, so we can't really do that.

We have a JSTraceable implementation for the HTML tokenizer in script/dom/servoparser/html.rs. I recommend following its example.

Er, but stripping out the unnecessary code leaves just:

#[allow(unsafe_code)] unsafe impl JSTraceable for HtmlTokenizer<PrefetchSink>> { unsafe fn trace(&self, trc: *mut JSTracer) { } }

which doesn't have obvious advantages over unsafe_no_jsmanaged_fields!.

Can't we still derive JStraceable on Sink and call self.sink.trace() at least?

The problem is we don't have access to the sink, since the tokenizer doesn't expose an API to access it. We couuuld use an Rc, but that seems a bit round-the-houses.

Isn't the tokenizer this type? It has a public sink field. What am I missing?

Argh, I was looking for an accessor method, I didn't spot that the field was public. D'oh!

Okay, fixed.

components/script/dom/servoparser/prefetch.rs

asajeffrey · 2019-09-06T15:20:31Z

I don't think there's security issues here, since the data requested by the prefetch is only used to populate the cache, and isn't exposed to script. If we do set the headers according to htmlscriptelement and htmlimageelement, then we do need to make sure we're not sending cookies we shouldn't.

asajeffrey · 2019-09-06T18:31:04Z

IRC chat at https://mozilla.logbot.info/servo/20190906#c16596185

asajeffrey · 2019-09-06T18:39:41Z

One thing I realized doing this is that fetch uses the origin, which is mutable, but for speculative prefetching we don't get much choice but to use the original origin.

jdm · 2019-09-06T22:09:31Z

@bors-servo try=wpt

bors-servo · 2019-09-06T22:09:36Z

⌛ Trying commit 330b940 with merge 419002d...


        Auto merge of #24148 - asajeffrey:script-prefetch, r=<try>

Prefetch img, scripts and stylesheets during parsing  Eagerly tokenize html input, and scan for `script` and `img` tags which can be prefetched into the cache. This allows resources to be loaded concurrently, which would otherwise be loaded sequentially due to being blocked by script execution. On the cloth webgl demo, this takes the time-to-paint down from 732ms to 560ms. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #3847 - [x] These changes do not require tests because it's a perf improvement    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24148)

bors-servo · 2019-09-10T17:51:49Z

⌛ Trying commit c6e1b8a with merge 8a20090...


        Auto merge of #24148 - asajeffrey:script-prefetch, r=<try>

Prefetch img, scripts and stylesheets during parsing  Eagerly tokenize html input, and scan for `script` and `img` tags which can be prefetched into the cache. This allows resources to be loaded concurrently, which would otherwise be loaded sequentially due to being blocked by script execution. On the cloth webgl demo, this takes the time-to-paint down from 732ms to 560ms. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #3847 - [x] These changes do not require tests because it's a perf improvement    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24148)

bors-servo · 2019-09-10T20:47:42Z

💔 Test failed - status-taskcluster

asajeffrey · 2019-09-10T20:52:31Z

OK, that was a CI failure, the WPT test suite passed!

asajeffrey · 2019-09-10T20:54:23Z

Now waiting for a review from @nox.

asajeffrey · 2019-09-10T20:56:16Z

IRC chat https://mozilla.logbot.info/servo/20190910#c16603991


        Prefetch img and scripts during parsing


        Make http_loader more robust against hyperium parse errors

asajeffrey · 2019-09-10T21:14:47Z

Squashed.

nox

I'm a bit anxious about doing that tbh, but it looks ok apart from some nits.

nox · 2019-09-11T08:02:56Z

components/net_traits/lib.rs

@@ -237,6 +237,18 @@ impl FetchTaskTarget for IpcSender<FetchResponseMsg> {
    }
 }

+impl FetchTaskTarget for () {


Make a new struct type, don't piggy-back the unit type. It's cheap to make a new one anyway.

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/mod.rs

@@ -101,6 +102,10 @@ pub struct ServoParser {
    aborted: Cell<bool>,
    /// <https://html.spec.whatwg.org/multipage/#script-created-parser>
    script_created_parser: bool,
+    /// We do a quick-and-dirty parse of the input looking for resources to prefetch


Let's write a big TODO that we should be carrying the speculative parsing PR to completion and remove that.

Done. The PR you mean is #19203?

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/mod.rs

@@ -425,20 +432,50 @@ impl ServoParser {
        )
    }

+    fn push_tendril_input_chunk(&self, chunk: StrTendril) {
+        if !chunk.is_empty() {


Early return if the chunk is empty instead of drifting towards the right.

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/mod.rs

    fn push_bytes_input_chunk(&self, chunk: Vec<u8>) {
+        // For byte input, we convert it to text using the network decoder.


Obvious comment is obvious. :)

Yes, but the matching comment on push_string_input_chunk wasn't (at least not to me!).

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/mod.rs

+                prefetch_input.push_back(chunk.clone());
+                let _ = self.prefetch_tokenizer
+                    .borrow_mut()
+                    .feed(&mut *prefetch_input);


The tokenizer here will be very wrong whenever someone uses document.write, I don't like that but I don't have any suggestion apart from "let's not land this PR and let's land speculative parsing instead".

Indeed. Speculative parsing will also be wrong in that case, not much you can do apart from add this to the list of reasons not to use document.write().

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/prefetch.rs

+}
+
+impl TokenSink for PrefetchSink {
+    type Handle = ();


Use a new unit struct type.

You are very keen on unit structs. Done.

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/prefetch.rs

+                },
+                (TagKind::EndTag, local_name!("script")) => {
+                    // After the first script tag, the main parser is blocked, so it's worth prefetching.
+                    self.prefetching = true;


I feel like that's the place where we should not prefetch ever, because the script blocking the parser may very well set window.domain or do stuff that makes this end tag's script not load or whatever.

Yeah, but if we waited to find out if script did any of that, we wouldn't be able to prefetch at all.

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/prefetch.rs

+                (TagKind::StartTag, local_name!("base")) => {
+                    if let Some(url) = self.get_url(tag, local_name!("href")) {
+                        debug!("Setting base {}", url);
+                        self.base = url;


This is extremely wrong in presence of multiple base tags in an document.

According to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base "There can be only one element in a document." But yes, I can imagine this causing resources to be prefetched erroneously.

OK, the spec has a note about this (https://html.spec.whatwg.org/multipage/semantics.html#the-base-element). Fixed.

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/prefetch.rs

+impl TokenSink for PrefetchSink {
+    type Handle = ();
+    fn process_token(&mut self, token: Token, _line_number: u64) -> TokenSinkResult<()> {
+        if let Token::TagToken(ref tag) = token {


Please early return out of the function if token isn't a TagToken, that will help with rightwards drift.

nox · 2019-09-11T08:02:56Z

components/script/dom/servoparser/prefetch.rs

+                            .send(CoreResourceMsg::Fetch(request, FetchChannels::Prefetch));
+                    }
+                    // Don't prefetch inside script
+                    self.prefetching = false;


Nothing but script data will be ever return from parsing a script, please use the proper TokenSinkResult return value to tell the tokenizer to switch modes.


        Responding to review comments

asajeffrey

OK, I think I addressed everything.

asajeffrey · 2019-09-11T16:41:19Z

components/net_traits/lib.rs

@@ -237,6 +237,18 @@ impl FetchTaskTarget for IpcSender<FetchResponseMsg> {
    }
 }

+impl FetchTaskTarget for () {


asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/mod.rs

@@ -101,6 +102,10 @@ pub struct ServoParser {
    aborted: Cell<bool>,
    /// <https://html.spec.whatwg.org/multipage/#script-created-parser>
    script_created_parser: bool,
+    /// We do a quick-and-dirty parse of the input looking for resources to prefetch


Done. The PR you mean is #19203?

asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/mod.rs

+                prefetch_input.push_back(chunk.clone());
+                let _ = self.prefetch_tokenizer
+                    .borrow_mut()
+                    .feed(&mut *prefetch_input);


Indeed. Speculative parsing will also be wrong in that case, not much you can do apart from add this to the list of reasons not to use document.write().

asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/mod.rs

    fn push_bytes_input_chunk(&self, chunk: Vec<u8>) {
+        // For byte input, we convert it to text using the network decoder.


Yes, but the matching comment on push_string_input_chunk wasn't (at least not to me!).

asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/mod.rs

-        self.network_input.borrow_mut().push_back(chunk.into());
+        // Convert the chunk to a tendril so cloning it isn't expensive.
+        // The input has already been decoded as a string, so doesn't need
+        // to be decoded by the network decoder again.


Not mentioning it was confusing to me though, I spent a while debugging before I realized I was re-encoding UTF-8 string data using the network decoder. I'll remove the comment about tendrils being cheap though, that's more me talking to myself than anything.

asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/prefetch.rs

+}
+
+impl TokenSink for PrefetchSink {
+    type Handle = ();


You are very keen on unit structs. Done.

asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/prefetch.rs

+impl TokenSink for PrefetchSink {
+    type Handle = ();
+    fn process_token(&mut self, token: Token, _line_number: u64) -> TokenSinkResult<()> {
+        if let Token::TagToken(ref tag) = token {


asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/prefetch.rs

+                            .send(CoreResourceMsg::Fetch(request, FetchChannels::Prefetch));
+                    }
+                    // Don't prefetch inside script
+                    self.prefetching = false;


asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/prefetch.rs

+                },
+                (TagKind::EndTag, local_name!("script")) => {
+                    // After the first script tag, the main parser is blocked, so it's worth prefetching.
+                    self.prefetching = true;


Yeah, but if we waited to find out if script did any of that, we wouldn't be able to prefetch at all.

asajeffrey · 2019-09-11T16:41:19Z

components/script/dom/servoparser/prefetch.rs

+                (TagKind::StartTag, local_name!("base")) => {
+                    if let Some(url) = self.get_url(tag, local_name!("href")) {
+                        debug!("Setting base {}", url);
+                        self.base = url;


According to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base "There can be only one element in a document." But yes, I can imagine this causing resources to be prefetched erroneously.


        Responding to review comments

asajeffrey · 2019-09-11T18:55:46Z

@nox, you okay with the changes?

nox · 2019-09-11T19:06:32Z

@bors-servo r=jdm,nox

bors-servo · 2019-09-11T19:06:33Z

📌 Commit 2e6f14f has been approved by jdm,nox


        Auto merge of #24148 - asajeffrey:script-prefetch, r=jdm,nox

Prefetch img, scripts and stylesheets during parsing  Eagerly tokenize html input, and scan for `script` and `img` tags which can be prefetched into the cache. This allows resources to be loaded concurrently, which would otherwise be loaded sequentially due to being blocked by script execution. On the cloth webgl demo, this takes the time-to-paint down from 732ms to 560ms. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #3847 - [x] These changes do not require tests because it's a perf improvement    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24148)

bors-servo · 2019-09-11T19:28:07Z

⌛ Testing commit 2e6f14f with merge 4e9967b...

bors-servo · 2019-09-11T22:18:59Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm,nox
Pushing 4e9967b to master...

asajeffrey added I-perf-slow A-content/script labels Sep 5, 2019

highfive assigned nox Sep 5, 2019

highfive added the S-awaiting-review label Sep 5, 2019

highfive assigned jdm and unassigned nox Sep 6, 2019

jdm requested changes Sep 6, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 6, 2019

asajeffrey added S-needs-squash S-awaiting-review and removed S-needs-code-changes labels Sep 6, 2019

asajeffrey changed the title ~~Prefetch img and scripts during parsing~~ Prefetch img, scripts and stylesheets during parsing Sep 6, 2019

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 6, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 6, 2019

jdm removed the S-awaiting-review label Sep 6, 2019

jdm approved these changes Sep 6, 2019

View changes

highfive added the S-tests-failed label Sep 10, 2019

asajeffrey added 2 commits Sep 5, 2019

Prefetch img and scripts during parsing

1aeb97b

Make http_loader more robust against hyperium parse errors

Loading status checks…

aeac382

asajeffrey force-pushed the asajeffrey:script-prefetch branch from c6e1b8a to aeac382 Sep 10, 2019

highfive removed the S-tests-failed label Sep 10, 2019

nox requested changes Sep 11, 2019

View changes

Responding to review comments

Loading status checks…

49a5e84

asajeffrey reviewed Sep 11, 2019

View changes

asajeffrey added the S-needs-squash label Sep 11, 2019

Responding to review comments

Loading status checks…

2e6f14f

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Sep 11, 2019

bors-servo merged commit 2e6f14f into servo:master Sep 11, 2019
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

Taskcluster (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 11, 2019

nox mentioned this pull request Sep 12, 2019

Off-main-thread, speculative HTML parsing #3847

Open

gterzian mentioned this pull request Sep 13, 2019

Impl speculative fetch #24207

Open

		fn push_bytes_input_chunk(&self, chunk: Vec<u8>) {
		// For byte input, we convert it to text using the network decoder.

servo / servo

Join GitHub today

Prefetch img, scripts and stylesheets during parsing #24148

Prefetch img, scripts and stylesheets during parsing #24148

Conversation

asajeffrey commented Sep 5, 2019 • edited by SimonSapin

highfive commented Sep 5, 2019

highfive commented Sep 5, 2019

asajeffrey commented Sep 5, 2019

asajeffrey commented Sep 6, 2019

asajeffrey commented Sep 6, 2019

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm Sep 6, 2019 • edited

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Sep 6, 2019

asajeffrey commented Sep 6, 2019

asajeffrey commented Sep 6, 2019

jdm commented Sep 6, 2019

bors-servo commented Sep 6, 2019

bors-servo commented Sep 10, 2019

bors-servo commented Sep 10, 2019

asajeffrey commented Sep 10, 2019

asajeffrey commented Sep 10, 2019

asajeffrey commented Sep 10, 2019

asajeffrey commented Sep 10, 2019

nox left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Sep 11, 2019

nox commented Sep 11, 2019

bors-servo commented Sep 11, 2019

bors-servo commented Sep 11, 2019

bors-servo commented Sep 11, 2019

asajeffrey commented Sep 5, 2019 •

edited by SimonSapin

jdm Sep 6, 2019 •

edited