added dom obj counting to decide sequential/parallel layout (#10110) #12862

notriddle · 2016-08-14T18:53:51Z

This is a rebased version of #11713

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10110 (github issue number if applicable).
There are no tests for these changes because it's an optimization with no visible behavioral changes

This change is

highfive · 2016-08-14T18:53:54Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/document.rs, components/script/parse/html.rs, components/script/dom/servohtmlparser.rs, components/script_layout_interface/message.rs, components/script/dom/window.rs

highfive · 2016-08-14T18:53:55Z

Warning

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

emilio · 2016-08-14T22:19:47Z

This was already reviewed in the original PR, so r=me,metajack I guess, though I think a check with browserhtml would be nice, to see if we parallelize in those cases.

@metajack what do you think about the following comments (excluding nits).

-S-awaiting-review +S-needs-code-changes

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

components/layout_thread/lib.rs, line 403 [r1] (raw file):

                                    thread_state::LAYOUT,
                                    layout_threads);

nit: extra newline.

components/layout_thread/lib.rs, line 741 [r1] (raw file):

        // ... as do each of the LayoutWorkers, if present.
        let ref traversal = self.parallel_traversal;

maybe merge these two lines? self.parallel_traversal.heap_size_of_tls()

components/layout_thread/lib.rs, line 802 [r1] (raw file):

    /// crash.
    fn exit_now(&mut self) {
        let ref mut traversal = self.parallel_traversal;

nit: self.parallel_traversal.shutdown()?

components/layout_thread/lib.rs, line 1458 [r1] (raw file):

                    let profiler_metadata = self.profiler_metadata();

                    debug!("layout: post style parallel? {}", self.parallel_flag);

Is this really needed?

components/script/dom/document.rs, line 415 [r1] (raw file):

    pub fn increment_dom_count(&self) {
        self.dom_count.set(self.dom_count.get() + 1);

Also, can you file a followup to update this when nodes are removed or inserted dynamically? Right now it seems to me that dom_count is something like dom_elements_at_first_page_parse, and it doesn't seem really nice not taking into account elements dinamically created. In fact, I doubt this will even turn parallel layout on for bhtml, for example, whose index.html page is almost empty.

components/script/dom/servohtmlparser.rs, line 284 [r1] (raw file):

};

nit: extra newline.

components/script/dom/servohtmlparser.rs, line 342 [r1] (raw file):

        &self.pending_input
    }

nit: So is this, and you can probably take rid of the other below.

components/script/parse/html.rs, line 94 [r1] (raw file):

        }

        self.document.increment_dom_count();

Should this also go in insert()? where text nodes are created?

Comments from Reviewable

notriddle · 2016-08-15T02:47:10Z

components/layout_thread/lib.rs, line 403 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

nit: extra newline.

Done.

Comments from Reviewable

notriddle · 2016-08-15T02:47:12Z

components/layout_thread/lib.rs, line 741 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

maybe merge these two lines? self.parallel_traversal.heap_size_of_tls()

Done.

Comments from Reviewable

notriddle · 2016-08-15T02:47:15Z

components/layout_thread/lib.rs, line 802 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

nit: self.parallel_traversal.shutdown()?

Done.

Comments from Reviewable

notriddle · 2016-08-15T02:47:17Z

components/layout_thread/lib.rs, line 1458 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Is this really needed?

Done.

Comments from Reviewable

notriddle · 2016-08-15T02:48:59Z

components/script/dom/servohtmlparser.rs, line 342 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

nit: So is this, and you can probably take rid of the other below.

Done.

Comments from Reviewable

notriddle · 2016-08-15T02:49:03Z

components/script/dom/servohtmlparser.rs, line 284 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

nit: extra newline.

Done.

Comments from Reviewable

notriddle · 2016-08-15T02:52:44Z

components/script/parse/html.rs, line 94 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Should this also go in insert()? where text nodes are created?

Text nodes can't be laid out in parallel, so a page with a bunch of text nodes but very little structure probably wouldn't benefit from parallel layout.

Comments from Reviewable

emilio · 2016-08-15T02:55:15Z

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

components/script/parse/html.rs, line 94 [r1] (raw file):

Previously, notriddle (Michael Howell) wrote…

Text nodes can't be laid out in parallel, so a page with a bunch of text nodes but very little structure probably wouldn't benefit from parallel layout.

They can be styled in parallel though, and styling is a decent amount of the whole layout process.

Comments from Reviewable

notriddle · 2016-08-15T02:59:07Z

components/script/dom/document.rs, line 415 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Also, can you file a followup to update this when nodes are removed or inserted dynamically? Right now it seems to me that dom_count is something like dom_elements_at_first_page_parse, and it doesn't seem really nice not taking into account elements dinamically created. In fact, I doubt this will even turn parallel layout on for bhtml, for example, whose index.html page is almost empty.

Done. #12871

Comments from Reviewable

notriddle · 2016-08-15T03:10:26Z

Review status: 4 of 6 files reviewed at latest revision, 2 unresolved discussions.

components/script/parse/html.rs, line 94 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

They can be styled in parallel though, and styling is a decent amount of the whole layout process.

Done.

Comments from Reviewable

nox · 2016-08-15T13:35:26Z

I think the counting should be a new weight field on Node and be updated on every DOM mutation. We already need to notify all ancestors on mutations that something in the tree changed, and I have a few ideas to use that weight as an optimisation. For example we know for sure that a node with a lesser weight than an other node cannot be one of its ancestors.

notriddle · 2016-08-16T01:12:17Z

In this PR, or a follow-up?

Comments from Reviewable

nox · 2016-08-16T07:16:49Z

@notriddle Yeah not blocking this.

emilio · 2016-08-16T17:48:12Z

r=me if you think testing with bhtml is not necessary, or if you have done it and it parallelizes/it's not a severe perf impact.

bors-servo · 2016-12-05T18:21:55Z

💔 Test failed - linux-rel-wpt


        Fix "a serious bug" in the `bind_to_tree` functions

notriddle · 2016-12-06T00:43:42Z

Let's see if that fixed it.

notriddle · 2016-12-06T00:43:52Z

@bors-servo try

bors-servo · 2016-12-06T00:43:56Z

⌛ Trying commit f8121ae with merge 6c9c14d...


        Auto merge of #12862 - servo:layout-new, r=<try>

added dom obj counting to decide sequential/parallel layout (#10110) This is a rebased version of #11713 --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10110 (github issue number if applicable). - [X] There are no tests for these changes because it's an optimization with no visible behavioral changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12862)

bors-servo · 2016-12-06T01:27:32Z

💔 Test failed - linux-rel-css

notriddle · 2016-12-06T01:29:20Z

I've finally managed to pass all the DOM tests. That fixed 'em.

  ▶ FAIL [expected PASS] /css21_dev/html4/abs-pos-non-replaced-vlr-149.htm
  └   → /css21_dev/html4/abs-pos-non-replaced-vlr-149.htm 495c94c5bdb9351da4864d1dffcb6741003f6a42
/css21_dev/html4/reference/abs-pos-non-replaced-vlr-009-ref.htm 760d37f04422ccc35f8b05dda55d0a8c5e6b2a3b
Testing 495c94c5bdb9351da4864d1dffcb6741003f6a42 == 760d37f04422ccc35f8b05dda55d0a8c5e6b2a3b

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/changing-while-transition.htm:
  └ PASS [expected FAIL] changing transition-property / values

Now for more slogging through reftests that fail in sequential mode.

emilio

Those are known intermittents:

r=me if tests pass and with the comment below addressed if you feel it's necessary :)

emilio · 2016-12-07T07:22:03Z

components/script/dom/htmltitleelement.rs

@@ -71,9 +71,12 @@ impl VirtualMethods for HTMLTitleElement {
        }
    }

-    fn bind_to_tree(&self, is_in_doc: bool) {
+    fn bind_to_tree(&self, tree_in_doc: bool) {
+        if let Some(ref s) = self.super_type() {


You can unwrap here, right? Also above.

emilio · 2016-12-07T07:22:29Z

@bors-servo try

KiChjang · 2016-12-07T09:12:30Z

@bors-servo retry

bors-servo · 2016-12-07T09:12:35Z

⌛ Trying commit f8121ae with merge 983a1f8...


        Auto merge of #12862 - servo:layout-new, r=<try>

added dom obj counting to decide sequential/parallel layout (#10110) This is a rebased version of #11713 --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10110 (github issue number if applicable). - [X] There are no tests for these changes because it's an optimization with no visible behavioral changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12862)

bors-servo · 2016-12-07T10:00:04Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

notriddle · 2016-12-07T14:39:54Z

That's not what the vast majority of the existing ones do.

@bors-servo r=emilio

bors-servo · 2016-12-07T14:39:56Z

📌 Commit f8121ae has been approved by emilio

notriddle · 2016-12-07T21:40:23Z

@bors-servo retry

bors-servo · 2016-12-07T22:32:21Z

⌛ Testing commit f8121ae with merge 0fe94a6...


        Auto merge of #12862 - servo:layout-new, r=emilio

added dom obj counting to decide sequential/parallel layout (#10110) This is a rebased version of #11713 --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10110 (github issue number if applicable). - [X] There are no tests for these changes because it's an optimization with no visible behavioral changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12862)

bors-servo · 2016-12-07T23:23:23Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned nox Aug 14, 2016

highfive added the S-awaiting-review label Aug 14, 2016

emilio added S-needs-code-changes and removed S-awaiting-review labels Aug 14, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 15, 2016

notriddle force-pushed the layout-new branch from b59e7fd to 59a2269 Aug 15, 2016

notriddle force-pushed the layout-new branch from 59a2269 to bb1b1db Aug 15, 2016

nox assigned emilio and unassigned nox Aug 16, 2016

KiChjang mentioned this pull request Aug 16, 2016

added dom obj counting to decide sequential/parallel layout (#10110) #11713

Closed

4 of 5 tasks complete

notriddle force-pushed the layout-new branch 2 times, most recently from 48b58ae to a0de902 Aug 17, 2016

highfive added the S-tests-failed label Dec 5, 2016

Fix "a serious bug" in the `bind_to_tree` functions

Loading status checks…

f8121ae

highfive removed the S-tests-failed label Dec 6, 2016

highfive added the S-tests-failed label Dec 6, 2016

emilio reviewed Dec 7, 2016

View changes

KiChjang removed the S-needs-rebase label Dec 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Dec 7, 2016

highfive removed the S-awaiting-merge label Dec 7, 2016

notriddle deleted the layout-new branch Dec 8, 2016

servo / servo

added dom obj counting to decide sequential/parallel layout (#10110) #12862

added dom obj counting to decide sequential/parallel layout (#10110) #12862

notriddle commented Aug 14, 2016 •

edited by larsbergstrom

highfive commented Aug 14, 2016

highfive commented Aug 14, 2016

emilio commented Aug 14, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

emilio commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

nox commented Aug 15, 2016

notriddle commented Aug 16, 2016

nox commented Aug 16, 2016

emilio commented Aug 16, 2016

bors-servo commented Dec 5, 2016

notriddle commented Dec 6, 2016

notriddle commented Dec 6, 2016

bors-servo commented Dec 6, 2016

bors-servo commented Dec 6, 2016

notriddle commented Dec 6, 2016 •

edited

emilio left a comment •

edited

This comment has been minimized.

emilio commented Dec 7, 2016

KiChjang commented Dec 7, 2016

bors-servo commented Dec 7, 2016

bors-servo commented Dec 7, 2016

notriddle commented Dec 7, 2016

bors-servo commented Dec 7, 2016

notriddle commented Dec 7, 2016

bors-servo commented Dec 7, 2016

bors-servo commented Dec 7, 2016

servo / servo

Join GitHub today

added dom obj counting to decide sequential/parallel layout (#10110) #12862

added dom obj counting to decide sequential/parallel layout (#10110) #12862

Conversation

notriddle commented Aug 14, 2016 • edited by larsbergstrom

highfive commented Aug 14, 2016

highfive commented Aug 14, 2016

emilio commented Aug 14, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

emilio commented Aug 15, 2016

notriddle commented Aug 15, 2016

notriddle commented Aug 15, 2016

nox commented Aug 15, 2016

notriddle commented Aug 16, 2016

nox commented Aug 16, 2016

emilio commented Aug 16, 2016

bors-servo commented Dec 5, 2016

notriddle commented Dec 6, 2016

notriddle commented Dec 6, 2016

bors-servo commented Dec 6, 2016

bors-servo commented Dec 6, 2016

notriddle commented Dec 6, 2016 • edited

emilio left a comment • edited

This comment has been minimized.

emilio Dec 7, 2016

emilio commented Dec 7, 2016

KiChjang commented Dec 7, 2016

bors-servo commented Dec 7, 2016

bors-servo commented Dec 7, 2016

notriddle commented Dec 7, 2016

bors-servo commented Dec 7, 2016

notriddle commented Dec 7, 2016

bors-servo commented Dec 7, 2016

bors-servo commented Dec 7, 2016

notriddle commented Aug 14, 2016 •

edited by larsbergstrom

notriddle commented Dec 6, 2016 •

edited

emilio left a comment •

edited