Separate interning text run #3369

djg · 2018-11-29T02:35:40Z

First part of separating Primitives into separate interned data.

This change is

gw3583 · 2018-11-29T21:04:58Z

Looks like there is a reftest failure on CI:

REFTEST TEST-UNEXPECTED-FAIL | reftests\text\shadow.yaml == reftests\text\shadow-ref.yaml | image comparison, max difference: 131, number of differing pixels: 6425

Any chance you could do a talos run before review, just to give an idea what effect it has on dl_mutate (even just to make sure it doesn't regress, for now, since we probably won't get big wins from this initial work)?

djg · 2018-11-29T21:25:01Z

Here is the try run with talos results. I see some small improvement and regression which is curious. https://treeherder.mozilla.org/#/jobs?repo=try&revision=752295ab260070f973c22dcb167cd2bef18e48bd

gw3583

Reviewed 6 of 12 files at r1.
Reviewable status: 6 of 12 files reviewed, 2 unresolved discussions (waiting on @djg)

webrender/src/clip.rs, line 101 at r1 (raw file):

#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub struct ClipUidMarker;
pub type ClipUid = intern::ItemUid<ClipUidMarker>;

This would become simpler if we use a global uid type (mentioned below).

webrender/src/intern.rs, line 76 at r1 (raw file):

/// Implement for a marker struct to associate the type of Uid marker to use
/// with `intern::Handle<..>`
pub trait UidMarker: Copy + Clone {

Having the item uids require this trait and phantom marker feels a bit overengineered to me. How about we just share one uid concrete type among all interners, and don't make them generic. That might aid in debugging too, since a uid is globally unique and would never overlap with a uid from a different interner type.

gw3583 · 2018-11-29T22:35:56Z

(partial review only so far, will work through this today).

bors-servo · 2018-11-29T23:59:26Z

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

djg · 2018-11-30T00:30:20Z

(partial review only so far, will work through this today).

Thanks @gw3583

djg

Reviewable status: 2 of 12 files reviewed, 2 unresolved discussions (waiting on @gw3583)

webrender/src/clip.rs, line 101 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

This would become simpler if we use a global uid type (mentioned below).

Done.

webrender/src/intern.rs, line 76 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Having the item uids require this trait and phantom marker feels a bit overengineered to me. How about we just share one uid concrete type among all interners, and don't make them generic. That might aid in debugging too, since a uid is globally unique and would never overlap with a uid from a different interner type.

Done.

gw3583

Reviewed 4 of 8 files at r2.
Reviewable status: 6 of 12 files reviewed, 2 unresolved discussions (waiting on @gw3583)

gw3583

Reviewed 1 of 12 files at r1, 1 of 8 files at r2.
Reviewable status: 8 of 12 files reviewed, 8 unresolved discussions (waiting on @gw3583 and @djg)

webrender/src/display_list_flattener.rs, line 1233 at r2 (raw file):

            );

            //cur_instance.kind = PrimitiveInstanceKind::Picture { pic_index: current_pic_index };

Why is this change needed?

webrender/src/display_list_flattener.rs, line 1264 at r2 (raw file):

            current_pic_index = filter_pic_index;
            //cur_instance.kind = PrimitiveInstanceKind::Picture { pic_index: current_pic_index };

same

webrender/src/display_list_flattener.rs, line 1302 at r2 (raw file):

            current_pic_index = blend_pic_index;
            // cur_instance.kind = PrimitiveInstanceKind::Picture { pic_index: blend_pic_index };
            match cur_instance.kind {

same

webrender/src/picture.rs, line 573 at r2 (raw file):

        );

        let prim_rect = match prim_instance.kind {

This is not quite right for Pictures (their rect is built dynamically), but this wasn't handled correctly previously anyway - I've got a fix for it in my local branch.

webrender/src/picture.rs, line 1253 at r2 (raw file):

            };

            let prim_data = match prim_instance.kind {

Maybe make this a helper method on FrameResources? Seems like it'll be used in a few places.

webrender/src/scene_builder.rs, line 173 at r2 (raw file):

}

// Access to `DocumentResources` interners by `Internable`

What is this trait needed for?

gw3583

Reviewed 1 of 12 files at r1, 3 of 8 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @gw3583 and @djg)

webrender/src/display_list_flattener.rs, line 2236 at r2 (raw file):

}

pub trait AsInstanceKind<H> {

Is there a reason to have these as separate traits rather than a "Primitive" trait or similar?

webrender/src/prim_store/text_run.rs, line 29 at r2 (raw file):

#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct TextRunKey {
    pub is_backface_visible: bool,

It's probably worth having a set of "common" structs that are embedded within each key / template etc for the data that is shared between all the different types?

webrender/src/prim_store/text_run.rs, line 80 at r2 (raw file):

#[cfg_attr(feature = "replay", derive(Deserialize))]
pub struct TextRunTemplate {
    pub is_backface_visible: bool,

Same here for common data.

djg

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @gw3583 and @djg)

webrender/src/display_list_flattener.rs, line 1233 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Why is this change needed?

Because PrimitiveInstanceKind:Picture now contains a data_handle. data_handle is Copy so I might try adding it the Picture { ... } here, instead of updating pic_index in-place.

djg

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @gw3583)

webrender/src/display_list_flattener.rs, line 2236 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Is there a reason to have these as separate traits rather than a "Primitive" trait or similar?

Because there implemented on different types. I can move BuildKey into the the Internable trait as is it related to generating the key for interning. AsInstanceKind is used for getting a PrimitiveInstanceKind and is implemented on the key that's the source, eg TextRunKey. CreateShadow and IsVisible are properties of the primitive and can be combined into a Primitive trait.

webrender/src/picture.rs, line 573 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

This is not quite right for Pictures (their rect is built dynamically), but this wasn't handled correctly previously anyway - I've got a fix for it in my local branch.

Do you need to post me the fix to incorporate?

webrender/src/picture.rs, line 1253 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Maybe make this a helper method on FrameResources? Seems like it'll be used in a few places.

Done.

webrender/src/scene_builder.rs, line 173 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

What is this trait needed for?

It's used by the generic version of DisplayListFlattener::create_primitive to obtain the correct interner for primitive.

        let interner = self.resources.interner_mut(); // Uses type deduction to work out the which store +
        let prim_data_handle =                        //                                                 |
            interner                                  //                                                 |
            .intern(&prim_key, || { ... });           // << From the types required here ----------------+

webrender/src/prim_store/text_run.rs, line 29 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

It's probably worth having a set of "common" structs that are embedded within each key / template etc for the data that is shared between all the different types?

OK, I had that in a version but on IRC you advised against it. I think having common structs can help clean up some pattern matching mess.

webrender/src/prim_store/text_run.rs, line 80 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Same here for common data.

Done.

djg

Reviewable status: 3 of 12 files reviewed, 11 unresolved discussions (waiting on @gw3583)

webrender/src/display_list_flattener.rs, line 2236 at r2 (raw file):

Previously, djg (Dan Glastonbury) wrote…

Because there implemented on different types. I can move BuildKey into the the Internable trait as is it related to generating the key for interning. AsInstanceKind is used for getting a PrimitiveInstanceKind and is implemented on the key that's the source, eg TextRunKey. CreateShadow and IsVisible are properties of the primitive and can be combined into a Primitive trait.

One thing I found that is having CreateShadow being a separate trait means that we can use the compiled to help us. ie.

impl CreateShadow for LinearGradient {
    fn create_shadow(&self, shadow: &Shadow) -> Self {
        PrimitiveKeyKind::LinearGradient { .. } |
        panic!("bug: this prim is not supported in shadow contexts");
    }
}

We don't need the panic! implementation and can have the compiler error when trying to create a shadow for LinearGradient because the CreateShadow trait doesn't exists for LinearGradient.

bors-servo · 2018-12-04T09:02:30Z

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

djg · 2018-12-05T00:09:25Z

@gw3583 Ping

gw3583 · 2018-12-05T00:17:44Z

Sorry, all-hands jetlag and that - I was going to take a more detailed look in the morning, but I think it probably looks good in general now, or the remaining bits could be done as follow ups. Did we work out what effect this has on talos?

gw3583 · 2018-12-05T00:19:28Z

Probably worth getting @kvark to take a quick look too, if he has time.

djg · 2018-12-05T00:29:43Z

I'm currently working through removing the conflict with servo/master. Update shortly.

@gw3583 It had a slight improvement on talos, but I wasn't expecting much of a change yet.

gw3583 · 2018-12-05T00:36:14Z

@djg Yea, I wouldn't expect a large improvement until we separate solid rects. As long as it doesn't regress, we're good for now 👍

bors-servo · 2018-12-05T04:18:56Z

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

gw3583 · 2018-12-05T16:00:28Z

Sorry, this got conflicts again - I think it should be good to go once those conflicts are resolved?

Only used by Primitive currently.

djg · 2018-12-05T23:57:28Z

Gecko try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b66118f88f9306e0d2fd6a94df9da24fb71a7135

djg · 2018-12-06T04:08:55Z

@gw3583 Apparently there is one comment to resolve?

gw3583 · 2018-12-06T13:37:20Z

I think it's fine to get this merged now, to avoid any more rebase issues. We should be able to fix up any remaining changes as follow ups.

gw3583 · 2018-12-06T13:37:30Z

@bors-servo r+

bors-servo · 2018-12-06T13:37:31Z

📌 Commit e3ccda7 has been approved by gw3583

bors-servo · 2018-12-06T14:45:56Z

⌛ Testing commit e3ccda7 with merge 6db2d6c...

Separate interning text run First part of separating Primitives into separate interned data.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3369)

bors-servo · 2018-12-06T15:55:41Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing 6db2d6c to master...

…449ec591527a (WR PR #3369). r=kats servo/webrender#3369 Differential Revision: https://phabricator.services.mozilla.com/D13936 --HG-- extra : moz-landing-system : lando

…449ec591527a (WR PR #3369). r=kats servo/webrender#3369 Differential Revision: https://phabricator.services.mozilla.com/D13936

…449ec591527a (WR PR #3369). r=kats servo/webrender#3369 Differential Revision: https://phabricator.services.mozilla.com/D13936 UltraBlame original commit: 690bea1cb27f8288c479bcea354474a17268ad96

…449ec591527a (WR PR #3369). r=kats servo/webrender#3369 Differential Revision: https://phabricator.services.mozilla.com/D13936

djg force-pushed the separate_interning_text_run branch from 6b3c530 to 823a663 Compare November 29, 2018 05:07

gw3583 requested changes Nov 29, 2018

View reviewed changes

djg force-pushed the separate_interning_text_run branch from 823a663 to 6497007 Compare November 30, 2018 03:08

djg commented Nov 30, 2018

View reviewed changes

gw3583 reviewed Nov 30, 2018

View reviewed changes

gw3583 requested changes Nov 30, 2018

View reviewed changes

gw3583 requested changes Dec 1, 2018

View reviewed changes

djg commented Dec 3, 2018

View reviewed changes

djg force-pushed the separate_interning_text_run branch from 6497007 to 6c697ab Compare December 3, 2018 05:04

djg commented Dec 3, 2018

View reviewed changes

djg force-pushed the separate_interning_text_run branch from 6c697ab to 3f368dc Compare December 5, 2018 01:51

gw3583 approved these changes Dec 5, 2018

View reviewed changes

djg added 4 commits December 6, 2018 08:41

Give intern::Interner an impl of Default.

87fe67e

Preparation for separate ItemUids.

d1d0d4a

Add support to generically add interned data to their interner.

280d07d

Only used by Primitive currently.

Move prim_store to a directory-based module.

7f6d2af

djg added 6 commits December 6, 2018 08:49

Extract TextRun data out of PrimitiveKeyKind.

6591e0a

Move all text related types into prim_store::text_run module.

c49cf4b

De-duplicate shadow item code.

8b9715e

Merge BuildKey trait with Internable trait.

30e29b8

Extract common key and template data.

ce799fb

Add PrimTemplateCommonData access helper to FrameResources.

e3ccda7

djg force-pushed the separate_interning_text_run branch from 3f368dc to e3ccda7 Compare December 5, 2018 23:38

bors-servo merged commit e3ccda7 into servo:master Dec 6, 2018

djg deleted the separate_interning_text_run branch December 7, 2018 05:45

Separate interning text run #3369

Separate interning text run #3369

Uh oh!

Conversation

djg commented Nov 29, 2018 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gw3583 commented Nov 29, 2018

Uh oh!

djg commented Nov 29, 2018

Uh oh!

gw3583 left a comment

Choose a reason for hiding this comment

Uh oh!

gw3583 commented Nov 29, 2018

Uh oh!

bors-servo commented Nov 29, 2018

Uh oh!

djg commented Nov 30, 2018

Uh oh!

djg left a comment

Choose a reason for hiding this comment

Uh oh!

gw3583 left a comment

Choose a reason for hiding this comment

Uh oh!

gw3583 left a comment

Choose a reason for hiding this comment

Uh oh!

gw3583 left a comment

Choose a reason for hiding this comment

Uh oh!

djg left a comment

Choose a reason for hiding this comment

Uh oh!

djg left a comment

Choose a reason for hiding this comment

Uh oh!

djg left a comment

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Dec 4, 2018

Uh oh!

djg commented Dec 5, 2018

Uh oh!

gw3583 commented Dec 5, 2018

Uh oh!

gw3583 commented Dec 5, 2018

Uh oh!

djg commented Dec 5, 2018

Uh oh!

gw3583 commented Dec 5, 2018

Uh oh!

bors-servo commented Dec 5, 2018

Uh oh!

gw3583 commented Dec 5, 2018

Uh oh!

djg commented Dec 5, 2018

Uh oh!

djg commented Dec 6, 2018

Uh oh!

gw3583 commented Dec 6, 2018

Uh oh!

gw3583 commented Dec 6, 2018

Uh oh!

bors-servo commented Dec 6, 2018

Uh oh!

bors-servo commented Dec 6, 2018

Uh oh!

bors-servo commented Dec 6, 2018

Uh oh!

Uh oh!

djg commented Nov 29, 2018 •

edited by larsbergstrom

Loading