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 Off-main-thread, speculative HTML parsing #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:48:21Z

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...

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...

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

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-11T07:46:27Z

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-11T07:50:43Z

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-11T07:51:29Z

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-11T07:51:39Z

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-11T07:53:03Z

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-11T07:55:07Z

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-11T07:56:53Z

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-11T07:57: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;


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:00:23Z

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:13Z

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.

asajeffrey

OK, I think I addressed everything.

asajeffrey · 2019-09-11T15:45:15Z

components/net_traits/lib.rs

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

+impl FetchTaskTarget for () {


asajeffrey · 2019-09-11T15:50:02Z

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-11T15:52:09Z

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-11T15:53:26Z

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-11T15:56:20Z

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:04:58Z

components/script/dom/servoparser/prefetch.rs

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


You are very keen on unit structs. Done.

asajeffrey · 2019-09-11T16:06:18Z

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:36:07Z

components/script/dom/servoparser/prefetch.rs

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


asajeffrey · 2019-09-11T16:37:32Z

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:39:39Z

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.

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

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 Unnecessary performance degredation. A-content/script Related to the script thread labels Sep 5, 2019

highfive assigned nox Sep 5, 2019

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 5, 2019

highfive assigned jdm and unassigned nox Sep 6, 2019

jdm requested changes Sep 6, 2019

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 6, 2019

asajeffrey added S-needs-squash Some (or all) of the commits in the PR should be combined. S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. 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 Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 6, 2019

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 6, 2019

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Sep 6, 2019

jdm approved these changes Sep 6, 2019

View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2019

Alan Jeffrey added 2 commits September 10, 2019 16:13

Prefetch img and scripts during parsing

1aeb97b

Make http_loader more robust against hyperium parse errors

aeac382

asajeffrey force-pushed the script-prefetch branch from c6e1b8a to aeac382 Compare September 10, 2019 21:13

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2019

nox suggested changes Sep 11, 2019

View reviewed changes

Responding to review comments

49a5e84

asajeffrey commented Sep 11, 2019

View reviewed changes

asajeffrey added the S-needs-squash Some (or all) of the commits in the PR should be combined. label Sep 11, 2019

Responding to review comments

2e6f14f

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Sep 11, 2019

bors-servo merged commit 2e6f14f into servo:master Sep 11, 2019

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. 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.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm Sep 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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