New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port text runs to be interned primitives. #3260

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Collaborator

gw3583 commented Nov 1, 2018

This makes the GPU cache for text runs stable across display
lists, which drastically reduces GPU cache updates on real
world pages when a new scene is built.

It also makes the prim_data_handle for text runs stable
across frames and display lists, making it very quick
to compare if a text run is equivalent.

Measurements of GPU cache rows (per profiler overlay):

Wikipedia front page: 82 -> 47
nytimes.com: 78 -> 56

I heaven't measured, but this should also reduce CPU compositor
times a bit, due to the reduced amount of updates to the GPU cache.


This change is Reviewable

@gw3583

This comment has been minimized.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 1, 2018

Try run looks good 🎉

@kvark

kvark approved these changes Nov 1, 2018

:lgtm: , a few questions

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gw3583)


webrender/src/prim_store.rs, line 1232 at r1 (raw file):

        raster_space: RasterSpace,
        display_list: &BuiltDisplayList,
        frame_building_state: &mut FrameBuildingState,

what's the motivation for removing the frame building state?


webrender/src/prim_store.rs, line 1739 at r1 (raw file):

    /// A run of glyphs, with associated font parameters.
    TextRun {
        run: TextRunPrimitive,

Can we get rid of TextRunPrimitive type completely now?


webrender/src/prim_store.rs, line 2836 at r1 (raw file):

        );

        self.opacity = match prim_data.kind {

I wonder if we could match both self.kind and prim_data.kind here? that would avoid the multiple nested matches with unreachable arms

Port text runs to be interned primitives.
This makes the GPU cache for text runs stable across display
lists, which drastically reduces GPU cache updates on real
world pages when a new scene is built.

It also makes the prim_data_handle for text runs stable
across frames and display lists, making it very quick
to compare if a text run is equivalent.

Measurements of GPU cache rows (per profiler overlay):

  Wikipedia front page:  82 -> 47
  nytimes.com:           78 -> 56
@gw3583

Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @kvark)


webrender/src/prim_store.rs, line 1232 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

what's the motivation for removing the frame building state?

It has a borrow check error due to the prim_data being borrowed above. Once the dust settles, I think that the prim_data borrows in this pass will end up being immutable, but for now this seems like the simplest option.


webrender/src/prim_store.rs, line 1739 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Can we get rid of TextRunPrimitive type completely now?

We can - I only left it to try and minimize the diff changes in this patch, but there's no technical reason to keep it as a separate type.


webrender/src/prim_store.rs, line 2836 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I wonder if we could match both self.kind and prim_data.kind here? that would avoid the multiple nested matches with unreachable arms

Indeed, that is much better - fixed.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 1, 2018

@kvark Thanks, all comments addressed, I think.

@kvark

kvark approved these changes Nov 1, 2018

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@kvark

This comment has been minimized.

Member

kvark commented Nov 1, 2018

thank you!
@bors-servo r+

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 1, 2018

📌 Commit d7ae71c has been approved by kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 1, 2018

⌛️ Testing commit d7ae71c with merge d30fda1...

bors-servo added a commit that referenced this pull request Nov 1, 2018

Auto merge of #3260 - gw3583:text-intern-4, r=kvark
Port text runs to be interned primitives.

This makes the GPU cache for text runs stable across display
lists, which drastically reduces GPU cache updates on real
world pages when a new scene is built.

It also makes the prim_data_handle for text runs stable
across frames and display lists, making it very quick
to compare if a text run is equivalent.

Measurements of GPU cache rows (per profiler overlay):

  Wikipedia front page:  82 -> 47
  nytimes.com:           78 -> 56

I heaven't measured, but this should also reduce CPU compositor
times a bit, due to the reduced amount of updates to the GPU cache.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3260)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 1, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing d30fda1 to master...

@bors-servo bors-servo merged commit d7ae71c into servo:master Nov 1, 2018

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 5 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment