Reorganise display list builder #20031

pyfisch · 2018-02-12T17:01:25Z

Description in the individual commits.

This change is

highfive · 2018-02-12T17:01:30Z

Heads up! This PR modifies the following files:

@emilio: components/layout/display_list/webrender_helpers.rs, components/layout/fragment.rs, components/layout/display_list/builder.rs, components/layout/display_list/mod.rs, components/layout/sequential.rs and 3 more

highfive · 2018-02-12T17:01:31Z

Warning

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

pyfisch · 2018-02-12T17:02:34Z

r? @mbrubeck

Now the gfx/display_list types are basically the same as the WebRender diplay items and could be replaced quite easily.

mbrubeck

This looks good, thanks! Some thoughts/comments below, which you can optionally address before landing.

mbrubeck · 2018-02-12T17:22:27Z

components/layout/display_list/builder.rs

+
+    // Returns the text index within a node for the point of interest.
+    pub fn text_index(&self, node: OpaqueNode, point_in_item: Point2D<Au>) -> Option<usize> {
+        if let Some(item) = self.inner.get(&node) {


Minor suggestion: You could use ? instead of if let ... else here.

Nice. Did not know it landed.

mbrubeck · 2018-02-12T17:27:57Z

components/layout/display_list/builder.rs

+        // Guard against duplicate nodes.
+        if self.inner.insert(node, item).is_some() {
+            debug!(
+                "TODO(#20020): Text indexing for {:?} is broken because of multiple text runs.",


Hmm... using a HashMap<OpaqueNode, IndexableTextItem> might make it harder to fix #20020. In the current code, all the data is in the display list, but the loop incorrectly stops at the first display item for the node. It seems like we could fix #20020 by continuing to search until we find the text run that actually contains the given advance. With the hash map, we don't even store all the text runs.

This doesn't need to be fixed right now, but what is the long-term plan? Maybe the values in the HashMap will change to something like SmallVec<[IndexableTextItem; 1]>?

I just realized that this also impacts #19754 which adds new code that queries for the text runs.

This doesn't need a full fix for #20020, but please at least change it to store all the text, rather than just the first fragment/run of each node, so that #19754 is still possible. Thanks!

mbrubeck · 2018-02-12T19:00:06Z

#19754 has been approved. If it merges successfully before this PR, you'll need to resolve some conflicts by changing its code that searches the display list for text items. I think this changes should be fairly straightforward, but I apoologize for the extra churn!

mbrubeck · 2018-02-12T19:01:26Z

There's a ./mach test-unit compile error in tests/unit/metrics/paint_time.rs related to the image_data change.

pyfisch · 2018-02-12T21:58:47Z

updated. But I guess #19754 will be merged first.

pyfisch · 2018-02-15T10:25:32Z

@bors-servo try

bors-servo · 2018-02-15T10:25:37Z

⌛ Trying commit ce65f22 with merge 6e01f26...

Reorganise display list builder Description in the individual commits.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20031)

bors-servo · 2018-02-15T10:45:52Z

💔 Test failed - linux-rel-wpt

pyfisch · 2018-02-15T11:35:54Z

Failures:

linux-rel-wpt LLVM ERROR: IO failure on output stream. unrelated
linux-rel-css {"status": "CRASH", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/syntax/parsing/html5lib_tests19.html?run_type=write", "line": 122589, "action": "test_result", "expected": "OK"} I think it is intermittent because of the crash report.

  ▶ CRASH [expected OK] /html/syntax/parsing/html5lib_tests19.html?run_type=write
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 17.3.0-devel
  │ failed to receive response to font request: Io(Os { code: 104, kind: ConnectionReset, message: "Connection reset by peer" }) (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(NonZero(NonZero(1))) }, at libcore/result.rs:916)
  │ stack backtrace:
  │    0:     0x7f7d954bb6bc - backtrace::backtrace::trace::hb73e73c3fa21d7b2
  │    1:     0x7f7d954ba862 - backtrace::capture::Backtrace::new::h8a585c120187c23d
  │    2:     0x7f7d92c1571f - servo::main::{{closure}}::he04abab9f980c87c
  │    3:     0x7f7d954c91f5 - std::panicking::rust_panic_with_hook::haa00f5a9417cd684
  │                         at libstd/panicking.rs:577
  │    4:     0x7f7d954c907e - std::panicking::begin_panic::ha956683198499384
  │                         at libstd/panicking.rs:537
  │    5:     0x7f7d954c8f7a - std::panicking::begin_panic_fmt::h587e116719f14631
  │                         at libstd/panicking.rs:521
  │    6:     0x7f7d954c8f12 - rust_begin_unwind
  │                         at libstd/panicking.rs:497
  │    7:     0x7f7d954fd6f0 - core::panicking::panic_fmt::h1d64949939b0af2f
  │                         at libcore/panicking.rs:71
  │    8:     0x7f7d9417fc6d - core::result::unwrap_failed::hc4c84a50ee7bb1c2
  │    9:     0x7f7d9417c1fb - gfx::font_cache_thread::FontCacheThread::last_resort_font_template::h53ebb917a6a9609c
  │   10:     0x7f7d9417d44c - gfx::font_context::FontContext::layout_font_group_for_style::haecb02b9f41e44ec
  │   11:     0x7f7d93eea869 - layout::text::TextRunScanner::scan_for_runs::h4c4c10ad985f215b
  │   12:     0x7f7d92e23afc - <std::thread::local::LocalKey<T>>::with::ha349b71481dddb4d
  │   13:     0x7f7d92f4d944 - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode>>::build_flow_for_block_starting_with_fragments::h4b8a89b5d8bcc202
  │   14:     0x7f7d92f4ae4a - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode>>::build_flow_for_block_like::h29eb69f7ca35f10e
  │   15:     0x7f7d92f4a5e7 - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode>>::build_flow_for_block::h9ff839decb3b0f34
  │   16:     0x7f7d92f333d2 - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode> as layout::traversal::PostorderNodeMutTraversal<ConcreteThreadSafeLayoutNode>>::process::h389a4c3f5bf628d7
  │   17:     0x7f7d92f2e9b9 - style::traversal::DomTraversal::handle_postorder_traversal::h3af9bdb5601dc593
  │   18:     0x7f7d92e5da29 - style::driver::traverse_dom::h85fa7d6a344b6c6a
  │   19:     0x7f7d92f5825c - profile_traits::time::profile::h9c132eb28cfeaeda
  │   20:     0x7f7d92e46ce1 - layout_thread::LayoutThread::handle_reflow::h847c5b2d6465f04e
  │   21:     0x7f7d92f56b2f - profile_traits::time::profile::h321a2638c96ebf25
  │   22:     0x7f7d92e40e7a - layout_thread::LayoutThread::handle_request_helper::h260f5a5ba2e8009b
  │   23:     0x7f7d92e3f879 - layout_thread::LayoutThread::start::h9b98f14ac82ac387
  │   24:     0x7f7d92f202a3 - profile_traits::mem::ProfilerChan::run_with_memory_reporting::he284b78be306f401
  │   25:     0x7f7d92f588fb - std::sys_common::backtrace::__rust_begin_short_backtrace::h02040504f43ec594
  │   26:     0x7f7d92e1e617 - _ZN3std9panicking3try7do_call17h6d40b2337f86c56cE.llvm.751C2F06
  │   27:     0x7f7d954f1f3e - __rust_maybe_catch_panic
  │                         at libpanic_unwind/lib.rs:102
  │   28:     0x7f7d92e55c7a - <F as alloc::boxed::FnBox<A>>::call_box::hba4ce46f0e0c7096
  │   29:     0x7f7d954dcc8b - <alloc::boxed::Box<alloc::boxed::FnBox<A, Output$u3d$R$GT$$u20$$u2b$$u20$$u27$a$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h13f1b0bf377a9f5b
  │                         at /checkout/src/liballoc/boxed.rs:798
  │                          - std::sys_common::thread::start_thread::hb5b6e0447b9691dc
  │                         at libstd/sys_common/thread.rs:24
  │                          - std::sys::unix::thread::Thread::new::thread_start::hcaff0bef1a01c624
  │                         at libstd/sys/unix/thread.rs:90
  │   30:     0x7f7d91afa183 - start_thread
  │   31:     0x7f7d903c203c - clone
  │   32:                0x0 - <unknown>
  │ ERROR:servo: failed to receive response to font request: Io(Os { code: 104, kind: ConnectionReset, message: "Connection reset by peer" })
  │ Pipeline failed in hard-fail mode.  Crashing!
  └ thread panicked while processing panic. aborting.

@mbrubeck I think this is good now.

pyfisch · 2018-02-21T11:26:16Z

@mbrubeck ping

bors-servo · 2018-02-21T18:13:50Z

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

mbrubeck

Sorry for the delay. Looks good, thanks! r=mbrubeck

It was unused.

Add an IndexableText structure for text queries. Instead of linear search for a node this now uses a HashMap. Remove the now irrelevant fields from TextDisplayItem.

Webrender solved the issue with servo/webrender#1189.

Store multiple indexable text items for a node.

pyfisch · 2018-02-24T10:09:20Z

Rebased.

mbrubeck · 2018-02-24T15:06:13Z

@bors-servo r+

bors-servo · 2018-02-24T15:06:13Z

📌 Commit 23fcd3a has been approved by mbrubeck

bors-servo · 2018-02-24T15:06:19Z

⌛ Testing commit 23fcd3a with merge 6a2feb3...

Reorganise display list builder Description in the individual commits.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20031)

bors-servo · 2018-02-24T15:55:49Z

☀️ 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: mbrubeck
Pushing 6a2feb3 to master...

highfive assigned glennw Feb 12, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 12, 2018

highfive assigned mbrubeck and unassigned glennw Feb 12, 2018

mbrubeck approved these changes Feb 12, 2018

View reviewed changes

mbrubeck 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 Feb 12, 2018

mbrubeck mentioned this pull request Feb 12, 2018

Implement element.innerText getter #19754

Merged

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 Feb 12, 2018

pyfisch force-pushed the transform branch from 35fef5a to ce65f22 Compare February 15, 2018 10:20

highfive added the S-tests-failed The changes caused existing tests to fail. label Feb 15, 2018

highfive added the S-needs-rebase There are merge conflict errors. label Feb 21, 2018

mbrubeck approved these changes Feb 21, 2018

View reviewed changes

pyfisch added 3 commits February 24, 2018 10:36

Use typed transforms in stacking contexts

279cda6

Remove image_data field from display item

a511513

It was unused.

Introduce a dedicated data structure for text queries

2d74bcf

Add an IndexableText structure for text queries. Instead of linear search for a node this now uses a HashMap. Remove the now irrelevant fields from TextDisplayItem.

pyfisch added 4 commits February 24, 2018 10:36

Remove now unnecessary fix_gradient_stops function

8d06111

Webrender solved the issue with servo/webrender#1189.

Move calculation functions out of builder.rs

0d0f2ab

Remove image_data from tests.

bc87b41

Store multiple indexable text items for a node.

Rebase and fix innerText

23fcd3a

pyfisch force-pushed the transform branch from ce65f22 to 23fcd3a Compare February 24, 2018 10:01

highfive removed the S-tests-failed The changes caused existing tests to fail. label Feb 24, 2018

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-rebase There are merge conflict errors. labels Feb 24, 2018

bors-servo merged commit 23fcd3a into servo:master Feb 24, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 24, 2018

pyfisch deleted the transform branch February 24, 2018 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganise display list builder #20031

Reorganise display list builder #20031

pyfisch commented Feb 12, 2018 •

edited by SimonSapin

Loading

highfive commented Feb 12, 2018

highfive commented Feb 12, 2018

pyfisch commented Feb 12, 2018

mbrubeck left a comment

mbrubeck Feb 12, 2018

pyfisch Feb 12, 2018

mbrubeck Feb 12, 2018

mbrubeck Feb 12, 2018

mbrubeck Feb 12, 2018

mbrubeck commented Feb 12, 2018

mbrubeck commented Feb 12, 2018

pyfisch commented Feb 12, 2018

pyfisch commented Feb 15, 2018

bors-servo commented Feb 15, 2018

bors-servo commented Feb 15, 2018

pyfisch commented Feb 15, 2018

pyfisch commented Feb 21, 2018

bors-servo commented Feb 21, 2018

mbrubeck left a comment

pyfisch commented Feb 24, 2018

mbrubeck commented Feb 24, 2018

bors-servo commented Feb 24, 2018

bors-servo commented Feb 24, 2018

bors-servo commented Feb 24, 2018

Reorganise display list builder #20031

Reorganise display list builder #20031

Conversation

pyfisch commented Feb 12, 2018 • edited by SimonSapin Loading

highfive commented Feb 12, 2018

highfive commented Feb 12, 2018

pyfisch commented Feb 12, 2018

mbrubeck left a comment

Choose a reason for hiding this comment

mbrubeck Feb 12, 2018

Choose a reason for hiding this comment

pyfisch Feb 12, 2018

Choose a reason for hiding this comment

mbrubeck Feb 12, 2018

Choose a reason for hiding this comment

mbrubeck Feb 12, 2018

Choose a reason for hiding this comment

mbrubeck Feb 12, 2018

Choose a reason for hiding this comment

mbrubeck commented Feb 12, 2018

mbrubeck commented Feb 12, 2018

pyfisch commented Feb 12, 2018

pyfisch commented Feb 15, 2018

bors-servo commented Feb 15, 2018

bors-servo commented Feb 15, 2018

pyfisch commented Feb 15, 2018

pyfisch commented Feb 21, 2018

bors-servo commented Feb 21, 2018

mbrubeck left a comment

Choose a reason for hiding this comment

pyfisch commented Feb 24, 2018

mbrubeck commented Feb 24, 2018

bors-servo commented Feb 24, 2018

bors-servo commented Feb 24, 2018

bors-servo commented Feb 24, 2018

pyfisch commented Feb 12, 2018 •

edited by SimonSapin

Loading