Servo's async Tokenizer: New! Improved! Now with multithreading! #17565

cynicaldevil · 2017-06-29T18:55:11Z

The Tokenizer, defined in the file async_html.rs will run on the main thread, while h5e's Tokenizer(along with its Sink) lives on the parser thread. Both h5e's Tokenizer and Sink communicate with the main thread Tokenizer via their own mpsc channels. The Sink keeps sending parser operations to the main thread, to be executed. For some operations, it waits for a message from the main thread before returning.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-06-29T18:55:15Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/servoparser/async_html.rs
@KiChjang: components/script/dom/servoparser/async_html.rs

highfive · 2017-06-29T18:55:16Z

Warning

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

cynicaldevil · 2017-06-29T19:18:06Z

cc @nox

cynicaldevil · 2017-06-29T19:18:21Z

components/script/dom/servoparser/async_html.rs

+}
+
+#[derive(HeapSizeOf)]
+enum ToSinkMsg {


The messages sent from the main thread Tokenizer to the Sink are the results of the synchronous operations we perform. Only two sync operations exist.

Add docs instead of GH comments. :)

Are these answers or questions?

cynicaldevil · 2017-06-29T19:18:21Z

components/script/dom/servoparser/async_html.rs

+            HtmlTokenizer(FromHtmlTokenizerMsg)
+        }
+
+        let mut send_tendrils = Vec::new();


Instead of making the BufferQueue implement Send, I now extract the StrTendrils from the BufferQueue and convert them to SendTendrils, which are then stored in a mutex. After we get a result from the feed function, we update input based on the contents of the mutex. The mutex is updated after HtmlTokenizer's feed method returns.

nox · 2017-07-01T10:44:40Z

51.24s$ ./mach test-compiletest
   Compiling compiletest_rs v0.2.6
   Compiling script v0.0.1 (file:///home/travis/build/servo/servo/components/script)
   Compiling compiletest_helper v0.0.1 (file:///home/travis/build/servo/servo/tests/compiletest/helper)
error[E0271]: type mismatch resolving `<html5ever::<unnamed>::fmt::UTF8 as html5ever::<unnamed>::fmt::SliceFormat>::Slice == html5ever::<unnamed>::SendTendril<html5ever::<unnamed>::fmt::UTF8>`
   --> /home/travis/build/servo/servo/components/script/dom/servoparser/async_html.rs:240:34
    |
240 |                         map(|st| StrTendril::from(st.clone())).collect::<VecDeque<StrTendril>>();
    |                                  ^^^^^^^^^^^^^^^^ expected str, found struct `html5ever::<unnamed>::SendTendril`
    |
    = note: expected type `str`
               found type `html5ever::<unnamed>::SendTendril<html5ever::<unnamed>::fmt::UTF8>`
    = note: required by `core::convert::From::from`
error[E0271]: type mismatch resolving `<html5ever::<unnamed>::fmt::UTF8 as html5ever::<unnamed>::fmt::SliceFormat>::Slice == html5ever::<unnamed>::SendTendril<html5ever::<unnamed>::fmt::UTF8>`
   --> /home/travis/build/servo/servo/components/script/dom/servoparser/async_html.rs:418:30
    |
418 |                     map(|st| StrTendril::from(st.clone())).collect::<VecDeque<StrTendril>>();
    |                              ^^^^^^^^^^^^^^^^ expected str, found struct `html5ever::<unnamed>::SendTendril`
    |
    = note: expected type `str`
               found type `html5ever::<unnamed>::SendTendril<html5ever::<unnamed>::fmt::UTF8>`
    = note: required by `core::convert::From::from`
error: aborting due to previous error(s)
error: Could not compile `script`.
To learn more, run the command again with --verbose.
The command "./mach test-compiletest" exited with 101.
50.57s$ ./mach test-unit
   Compiling size_of_test v0.0.1 (file:///home/travis/build/servo/servo/components/size_of_test)
   Compiling servo_remutex_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/servo_remutex)
   Compiling servo_config_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/servo_config)
   Compiling net_traits_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/net_traits)
   Compiling selectors v0.19.0 (file:///home/travis/build/servo/servo/components/selectors)
   Compiling profile_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/profile)
   Compiling script v0.0.1 (file:///home/travis/build/servo/servo/components/script)
   Compiling net_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/net)
   Compiling gfx_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/gfx)
warning: unused import: `std::borrow::Cow`
    --> /home/travis/build/servo/servo/components/selectors/parser.rs:1575:9
     |
1575 |     use std::borrow::Cow;
     |         ^^^^^^^^^^^^^^^^
     |
     = note: #[warn(unused_imports)] on by default
   Compiling layout_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/layout)
error[E0271]: type mismatch resolving `<html5ever::<unnamed>::fmt::UTF8 as html5ever::<unnamed>::fmt::SliceFormat>::Slice == html5ever::<unnamed>::SendTendril<html5ever::<unnamed>::fmt::UTF8>`
   --> /home/travis/build/servo/servo/components/script/dom/servoparser/async_html.rs:240:34
    |
240 |                         map(|st| StrTendril::from(st.clone())).collect::<VecDeque<StrTendril>>();
    |                                  ^^^^^^^^^^^^^^^^ expected str, found struct `html5ever::<unnamed>::SendTendril`
    |
    = note: expected type `str`
               found type `html5ever::<unnamed>::SendTendril<html5ever::<unnamed>::fmt::UTF8>`
    = note: required by `core::convert::From::from`
error[E0271]: type mismatch resolving `<html5ever::<unnamed>::fmt::UTF8 as html5ever::<unnamed>::fmt::SliceFormat>::Slice == html5ever::<unnamed>::SendTendril<html5ever::<unnamed>::fmt::UTF8>`
   --> /home/travis/build/servo/servo/components/script/dom/servoparser/async_html.rs:418:30
    |
418 |                     map(|st| StrTendril::from(st.clone())).collect::<VecDeque<StrTendril>>();
    |                              ^^^^^^^^^^^^^^^^ expected str, found struct `html5ever::<unnamed>::SendTendril`
    |
    = note: expected type `str`
               found type `html5ever::<unnamed>::SendTendril<html5ever::<unnamed>::fmt::UTF8>`
    = note: required by `core::convert::From::from`
error: aborting due to previous error(s)
error: Could not compile `script`.
To learn more, run the command again with --verbose.
The command "./mach test-unit" exited with 101.

cynicaldevil · 2017-07-01T12:26:57Z

Yes, it requires the new tendril version, which implements clone for Sendtendril. It works, I have compiled this locally using that.

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

@@ -4,12 +4,13 @@

 #![allow(unrooted_must_root)]

+use core::iter::FromIterator;
+use core::str::FromStr;


Aren't these already imported? At the very least, you shouldn't depend on them from core.

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

-        }
-    }
+#[derive(HeapSizeOf, JSTraceable)]
+struct Attribute {


Why is this now needed?

The attributes defined in h5e can't be sent between threads, so I convert them to this type before sending them.

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

 enum ParseOperation {
    GetTemplateContents(ParseNodeId, ParseNodeId),
-    CreateElement(ParseNodeId, QualName, Vec<Attribute>),
-    CreateComment(StrTendril, ParseNodeId),
+    CreateElement(ParseNodeId, QualName, Vec<Attribute>, u64),


Please make these have named fields, as I mentioned in the first PR. What even is that u64?

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

    Pop(ParseNodeId),
+    SetQuirksMode(
+        #[ignore_heap_size_of = "Defined in style"]
+        ServoQuirksMode


Derive HeapSizeOf on ServoQuirksMode in style.

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

 }

-#[derive(JSTraceable, HeapSizeOf)]
+#[derive(HeapSizeOf)]
+enum FromHtmlTokenizerMsg {


Don't use an enum if there is only a single variant.

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

+}
+
+#[derive(HeapSizeOf)]
+enum ToSinkMsg {


Add docs instead of GH comments. :)

Are these answers or questions?

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

+        let send_tendrils = Arc::new(Mutex::new(send_tendrils));
+        // Send message to parser thread, asking it to started reading from the shared input.
+        // Parser operation messages will be sent to main thread as they are evaluated.
+        self.html_tokenizer_sender.send(ToHtmlTokenizerMsg::Feed(send_tendrils.clone())).unwrap();


Why do we need to put the tendrils in a Arc<Mutex<T>> if we just send them afterwards?

I need to access send_tendrils again a few lines below....but this seems confusing to others. I think I'll remove the mutex and just pass it in the message itself.

Hmm...yeah... passing them via messages is not possible, needs HeapSizeOf impl for Tendril. Mutexes will have to do for now.

HeapSizeOf can be ignored, I think it's less important than avoiding mutices were we can.

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

+        match receiver.recv().expect("Unexpected sink channel panic in Html parser thread") {
+            ToHtmlTokenizerMsg::Feed(data) => {
+                let mut data = data.lock().unwrap();
+                let input = (*data).iter().


Why the explicit deref here?

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

+            ToHtmlTokenizerMsg::Feed(data) => {
+                let mut data = data.lock().unwrap();
+                let input = (*data).iter().
+                    map(|st| StrTendril::from(st.clone())).collect::<VecDeque<StrTendril>>();


Why the VecDeque here?

nox · 2017-07-03T10:18:26Z

components/script/dom/servoparser/async_html.rs

+    is_integration_point: bool,
+}
+
+impl Default for ParseNodeData {


This can be derived.

nox · 2017-07-03T10:19:17Z

Yes, it requires the new tendril version, which implements clone for Sendtendril. It works, I have compiled this locally using that.

Use tendril 0.3.1.

cynicaldevil · 2017-07-05T21:09:20Z

Currently, tendril's types are imported via h5e, for example, StrTendril is imported as use html5ever::tendril::StrTendril, so the tendril version in h5e needs to be updated first, followed by a h5e version update in Servo.

cynicaldevil · 2017-07-06T15:59:48Z

@nox I've updated the PR with the changes. I also added some comments to better explain the workings.

Now, if we ask bors to try this PR, it will test it using the synchronous tokenizer. Should I add an additional commit which enables the async tokenizer in preferences, just for testing purposes?

bors-servo · 2017-07-06T20:05:52Z

☔ The latest upstream changes (presumably #17624) made this pull request unmergeable. Please resolve the merge conflicts.

cynicaldevil · 2017-07-09T09:58:41Z

I rebased the PR, but don't know why the needs-rebase label wasn't removed.

bors-servo · 2017-07-11T02:18:05Z

☔ The latest upstream changes (presumably #17655) made this pull request unmergeable. Please resolve the merge conflicts.

cynicaldevil · 2017-07-11T15:39:10Z

@nox ready for try, then?

KiChjang · 2017-07-11T19:12:16Z

@bors-servo try

bors-servo · 2017-07-11T19:12:22Z

⌛ Trying commit 5fae497 with merge ff48c7f...


        Auto merge of #17565 - cynicaldevil:new-parser-thread, r=<try>

Servo's async Tokenizer: New! Improved! Now with multithreading!  **Note: Not to be merged until tendril v0.3.1 lands in Servo.** The Tokenizer, defined in the file `async_html.rs` will run on the main thread, while h5e's `Tokenizer`(along with its Sink) lives on the parser thread. Both h5e's `Tokenizer` and `Sink` communicate with the main thread Tokenizer via their own mpsc channels. The Sink keeps sending parser operations to the main thread, to be executed. For some operations, it waits for a message from the main thread before returning. ---  - [ ] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17565)

bors-servo · 2017-07-11T19:45:54Z

💔 Test failed - mac-rel-wpt4

cynicaldevil · 2017-07-12T15:54:42Z

@nox Removed a sender-receiver pair and merged it with another one. Now HtmlTokenizer and Sink both send messages to the same receiver. This solves the concurrency issue which was causing the tests to fail, and also makes it a little easier to understand the whole thing.

@bors-servo try

cynicaldevil · 2017-07-28T14:41:09Z

@bors-servo try

bors-servo · 2017-07-28T14:41:14Z

⌛ Trying commit b33aef2 with merge b98aefc...


        Auto merge of #17565 - cynicaldevil:new-parser-thread, r=<try>

Servo's async Tokenizer: New! Improved! Now with multithreading!  The Tokenizer, defined in the file `async_html.rs` will run on the main thread, while h5e's `Tokenizer`(along with its Sink) lives on the parser thread. Both h5e's `Tokenizer` and `Sink` communicate with the main thread Tokenizer via their own mpsc channels. The Sink keeps sending parser operations to the main thread, to be executed. For some operations, it waits for a message from the main thread before returning. ---  - [ ] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17565)

bors-servo · 2017-07-28T15:20:25Z

💔 Test failed - mac-rel-wpt4

highfive · 2017-07-28T15:20:27Z

  ▶ Unexpected subtest result in /html/dom/dynamic-markup-insertion/document-write/iframe_005.html:
  └ PASS [expected FAIL] document.write external script into iframe write back into parent

  ▶ Unexpected subtest result in /html/dom/dynamic-markup-insertion/opening-the-input-stream/010.html:
  └ PASS [expected FAIL] Salvagability of document.opened document


        Run a subset of tests using async html tokenizer

cynicaldevil · 2017-07-28T15:55:48Z

Forgot to add changes before committing, trying again:
@bors-servo try

bors-servo · 2017-07-28T15:55:53Z

⌛ Trying commit 0750fdc with merge 98653c9...


        Auto merge of #17565 - cynicaldevil:new-parser-thread, r=<try>

Servo's async Tokenizer: New! Improved! Now with multithreading!  The Tokenizer, defined in the file `async_html.rs` will run on the main thread, while h5e's `Tokenizer`(along with its Sink) lives on the parser thread. Both h5e's `Tokenizer` and `Sink` communicate with the main thread Tokenizer via their own mpsc channels. The Sink keeps sending parser operations to the main thread, to be executed. For some operations, it waits for a message from the main thread before returning. ---  - [ ] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17565)

bors-servo · 2017-07-28T16:50:50Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

jdm · 2017-07-28T17:23:27Z

@bors-servo: r=nox,jdm

bors-servo · 2017-07-28T17:23:27Z

📌 Commit 0750fdc has been approved by nox,jdm

bors-servo · 2017-07-28T17:23:31Z

⌛ Testing commit 0750fdc with merge e8272dc...


        Auto merge of #17565 - cynicaldevil:new-parser-thread, r=nox,jdm

Servo's async Tokenizer: New! Improved! Now with multithreading!  The Tokenizer, defined in the file `async_html.rs` will run on the main thread, while h5e's `Tokenizer`(along with its Sink) lives on the parser thread. Both h5e's `Tokenizer` and `Sink` communicate with the main thread Tokenizer via their own mpsc channels. The Sink keeps sending parser operations to the main thread, to be executed. For some operations, it waits for a message from the main thread before returning. ---  - [ ] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17565)

bors-servo · 2017-07-28T18:26:48Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox,jdm
Pushing e8272dc to master...


        Backed out changeset a417b9d7712d for vendoring bustage. r=backout on…

… a CLOSED TREE Backs out servo#17565


        Auto merge of #17910 - moz-servo-sync:gecko-backout, r=moz-servo-sync

Backed out changeset a417b9d7712d for vendoring bustage. r=backout on a CLOSED TREE Backed out changeset a417b9d7712d for vendoring bustage. r=backout on a CLOSED TREE Backs out #17565


        Auto merge of #17910 - moz-servo-sync:gecko-backout, r=moz-servo-sync

Multiple gecko backouts Backed out changeset a417b9d7712d for vendoring bustage. r=backout on a CLOSED TREE Backs out #17565  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17910)  --- Backed out changeset c424ad1c5f94 for build failures a=backout CLOSED TREE Backs out #17892

jdm · 2017-07-29T05:59:10Z

This was reverted by Gecko and needs to be merged again. However, to avoid the problem that triggered the revert, we probably need to conditionally add HeapSizeOf in selectors if a "servo" feature is enabled for the crate.


        Auto merge of #17914 - cynicaldevil:new-parser-thread, r=emilio

Run the async HTML Tokenizer on a new thread  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors Follow up for #17565    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17914)

highfive assigned nox Jun 29, 2017

highfive added the S-awaiting-review label Jun 29, 2017

cynicaldevil reviewed Jun 29, 2017

View changes

nox requested changes Jul 3, 2017

View changes

cynicaldevil force-pushed the cynicaldevil:new-parser-thread branch from e42747d to ef283e5 Jul 6, 2017

highfive added the S-needs-rebase label Jul 6, 2017

cynicaldevil force-pushed the cynicaldevil:new-parser-thread branch from ef283e5 to bb05fd0 Jul 9, 2017

KiChjang removed the S-needs-rebase label Jul 9, 2017

highfive added the S-needs-rebase label Jul 11, 2017

cynicaldevil force-pushed the cynicaldevil:new-parser-thread branch from bb05fd0 to 5fae497 Jul 11, 2017

highfive added the S-tests-failed label Jul 11, 2017

cynicaldevil force-pushed the cynicaldevil:new-parser-thread branch from 5fae497 to aebd6d4 Jul 12, 2017

highfive removed the S-tests-failed label Jul 12, 2017

highfive added the S-awaiting-review label Jul 28, 2017

highfive added the S-tests-failed label Jul 28, 2017

Run a subset of tests using async html tokenizer

Loading status checks…

0750fdc

cynicaldevil force-pushed the cynicaldevil:new-parser-thread branch from b33aef2 to 0750fdc Jul 28, 2017

highfive removed the S-tests-failed label Jul 28, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 28, 2017

bors-servo merged commit 0750fdc into servo:master Jul 28, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 28, 2017

moz-servo-sync mentioned this pull request Jul 28, 2017

Multiple gecko backouts #17910

Merged

moz-servo-sync added a commit to moz-servo-sync/servo that referenced this pull request Jul 28, 2017

Backed out changeset a417b9d7712d for vendoring bustage. r=backout on…

f022936

… a CLOSED TREE Backs out servo#17565

cynicaldevil mentioned this pull request Jul 29, 2017

Run the async HTML Tokenizer on a new thread #17914

Merged

2 of 2 tasks complete

servo / servo

Join GitHub today

Servo's async Tokenizer: New! Improved! Now with multithreading! #17565

Servo's async Tokenizer: New! Improved! Now with multithreading! #17565

Conversation

cynicaldevil commented Jun 29, 2017 • edited

highfive commented Jun 29, 2017

highfive commented Jun 29, 2017

cynicaldevil commented Jun 29, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Jul 1, 2017

cynicaldevil commented Jul 1, 2017

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.

nox commented Jul 3, 2017

cynicaldevil commented Jul 5, 2017

cynicaldevil commented Jul 6, 2017

bors-servo commented Jul 6, 2017

cynicaldevil commented Jul 9, 2017

bors-servo commented Jul 11, 2017

cynicaldevil commented Jul 11, 2017

KiChjang commented Jul 11, 2017

bors-servo commented Jul 11, 2017

bors-servo commented Jul 11, 2017

cynicaldevil commented Jul 12, 2017

cynicaldevil commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

highfive commented Jul 28, 2017

cynicaldevil commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

jdm commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

jdm commented Jul 29, 2017

cynicaldevil commented Jun 29, 2017 •

edited