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

[avadacatavra]

Added dom count to documents to track number of objects in dom. Changed layout_thread to always create the thread pool, then check to see if it should be parallel/or sequential when it's used.

---

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

• There are tests for these changes OR
• These changes do not require tests because @metajack and I aren't sure how to

---

This change is

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/document.rs, components/script/parse/html.rs, components/script/layout_interface.rs, components/script/dom/window.rs, components/script/dom/servohtmlparser.rs

[highfive]

Warning

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

[metajack]

One small omission (-y 1 has changed semantics with this patch and shouldn't) and the rest are nits. r=me with those fixes.

Previously, highfive wrote…

Warning

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

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

components/layout/layout_thread.rs, line 790 [r1] (raw file):

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

Don't add extra space after =. One space only.

---

components/layout/layout_thread.rs, line 1009 [r1] (raw file):

        let document = document.as_document().unwrap();

        self.parallel_flag = data.dom_count > 600;

This needs to take the layout threads count into consideration. We don't want a parallel traversal on a thread pool with 1 thread in it, we want the single threaded traversal.

Maybe self.parallel_flag = opts::get().layout_threads > 1 && data.dom_count > 600?

Also, where does 600 come from? @rzambre's suggestion was 750 based on his data. This probably needs a comment documenting where this value came from as well. A link to the bug is sufficient.

---

components/layout/layout_thread.rs, line 1136 [r1] (raw file):

                    || {
                // Perform CSS selector matching and flow construction.
                match self.parallel_flag {

Please turn this into a normal if now that's it's a boolean and not an Option.

---

components/layout/layout_thread.rs, line 1138 [r1] (raw file):

                match self.parallel_flag {
                    false => {
                        //Sequential mode

Space after // and ///. Does tidy not catch that?

---

components/layout/layout_thread.rs, line 1144 [r1] (raw file):

                    true => {
                        //Parallel mode
                        let ref mut traversal = self.parallel_traversal;

No need for let here. Just pass &mut self.parallel_traversal directly below.

---

components/layout/layout_thread.rs, line 1403 [r1] (raw file):

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

Normal if please.

---

components/layout/layout_thread.rs, line 1410 [r1] (raw file):

                        true => {
                            // Parallel mode.
                            let ref mut traversal = self.parallel_traversal;

As above, skip the let binding and just pass &mut self.parallel_traversal directly.

---

components/layout/layout_thread.rs, line 1418 [r1] (raw file):

                        }
                    }

Please don't add new blank lines unless near code you changed.

---

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

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

Spaces around +.

---

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

        };

Don't add blank lines here. There are no non-whitespace changes in this file, so just revert these two blank line additions.

---

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

}

Spurious blank line.

---

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

    type Handle = JS<Node>;

Ditto.

---

Comments from Reviewable

[highfive]

New code was committed to pull request.

[avadacatavra]

r? @metajack

[highfive]

New code was committed to pull request.

[highfive]

New code was committed to pull request.

[metajack]

One last nit: You still have no space betwen // and Sequential mode

Previously, highfive wrote…

New code was committed to pull request.

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[highfive]

New code was committed to pull request.

[metajack]

@bors-servo r+

This looks great. It will be interesting to see if we see a decrease in test times since most of the wpt tests probably fall into the tiny DOM tree category.

Previously, highfive wrote…

New code was committed to pull request.

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bors-servo]

📌 Commit 824b499 has been approved by metajack

[bors-servo]

⌛ Testing commit 824b499 with merge 900ff71...

[bors-servo]

💔 Test failed - windows

[metajack]

@bors-servo retry

[larsbergstrom]

@bors-servo retry

• #11719

[metajack]

@bors-servo p=2

will help in london

[bors-servo]

💔 Test failed - mac-rel-wpt

[highfive]

  ▶ FAIL [expected PASS] /html/rendering/non-replaced-elements/flow-content-0/div-align.html
  └   → /html/rendering/non-replaced-elements/flow-content-0/div-align.html 0fa0d794dc32120bbc32a401c0bf612808148976
/html/rendering/non-replaced-elements/flow-content-0/div-align-ref.html 158d8fe8cbec2c3121f56130ef092d51a21491ef
Testing 0fa0d794dc32120bbc32a401c0bf612808148976 == 158d8fe8cbec2c3121f56130ef092d51a21491ef

  ▶ FAIL [expected PASS] /_mozilla/css/overconstrained_block.html
  └   → /_mozilla/css/overconstrained_block.html 16bde43a535ed508af41c165882e9e909fa79450
/_mozilla/css/overconstrained_block_ref.html 0c1ff3395cd1542c73375a3b674286f1380893c4
Testing 16bde43a535ed508af41c165882e9e909fa79450 == 0c1ff3395cd1542c73375a3b674286f1380893c4

[bors-servo]

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

[KiChjang]

What's the status of this? @mbrubeck is on leave now, so who's owning this PR?

[KiChjang]

Oh, superseded by #12862.