Skip to content
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

Improve scene building and frame building performance by reducing allocations #3117

Merged
merged 3 commits into from Sep 25, 2018

Conversation

@mattwoodrow
Copy link
Contributor

mattwoodrow commented Sep 25, 2018

This roughly triples our FPS on the displaylist_mutate talos test, and goes a long way towards resolving #3116.


This change is Reviewable

@mattwoodrow mattwoodrow changed the title Improve scene building and frame building performance by reducing allocation Improve scene building and frame building performance by reducing allocations Sep 25, 2018
@mattwoodrow mattwoodrow force-pushed the mattwoodrow:master branch from e9e49f0 to 8ce8f9a Sep 25, 2018
@@ -603,6 +609,7 @@ impl<'de> Deserialize<'de> for BuiltDisplayList {
send_start_time: 0,
total_clip_nodes,
total_spatial_nodes,
prim_count_estimate : 0,

This comment has been minimized.

@zptan

zptan Sep 25, 2018

Unnecessary space before ':'

@mattwoodrow mattwoodrow force-pushed the mattwoodrow:master branch 2 times, most recently from db48b42 to 3721810 Sep 25, 2018
Copy link
Collaborator

gw3583 left a comment

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


webrender/src/border.rs, line 686 at r1 (raw file):

        widths: &LayoutSideOffsets,
        scale: LayoutToDeviceScale,
        brush_segments: &mut SmallVec<[BrushSegment; 4]>,

It might be worth making the size of this 8 - that should never spill to the heap then, since there will never be more than 4 corners + 4 edge segments added.

nit: Possibly worth using a type so the constant isn't repeated, e.g:
type BrushSegmentVec = SmallVec<[BrushSegment; 8]>;


___
*[webrender/src/display_list_flattener.rs, line 299 at r1](https://reviewable.io/reviews/servo/webrender/3117#-LNDs-d_B7HmERCXdiHF:-LNDs-d_B7HmERCXdiHG:b-5g6aho) ([raw file](https://github.com/servo/webrender/blob/3721810684d9caba5da3f4211f1e2da1d3426bae/webrender/src/display_list_flattener.rs#L299)):*
> ```Rust
>         self.flatten_items(&mut pipeline.display_list.iter(), pipeline_id, LayoutVector2D::zero());
> 
>         self.prim_count_estimate -= pipeline.display_list.prim_count_estimate();
> ```

I don't quite follow why we subtract here?
___
*[webrender/src/segment.rs, line 193 at r1](https://reviewable.io/reviews/servo/webrender/3117#-LNDscVB2J-z1V8xmCtw:-LNDscVB2J-z1V8xmCtx:bff7tgz) ([raw file](https://github.com/servo/webrender/blob/3721810684d9caba5da3f4211f1e2da1d3426bae/webrender/src/segment.rs#L193)):*
> ```Rust
>     }
> 
>     pub fn initialize(
> ```

Since we're re-using the segment builder now, perhaps we should add a debug bool to the struct that tracks initialization, to catch any misuse?

Something like:
 - set bool to false on construction, and after build() is called
 - set to true by initialize()
 - assert that it's true at the start of build() ?

I'm not sure if it's actually worth it, thoughts? 


<!-- Sent from Reviewable.io -->
…ing and pre-allocate the vector to that size
@mattwoodrow mattwoodrow force-pushed the mattwoodrow:master branch 2 times, most recently from 4bb9ca4 to 69740d5 Sep 25, 2018
@gw3583
Copy link
Collaborator

gw3583 commented Sep 25, 2018

Looks good! r=me once CI passes.

@gw3583
gw3583 approved these changes Sep 25, 2018
@gw3583
Copy link
Collaborator

gw3583 commented Sep 25, 2018

Oh, some compile errors in release builds. Must be missing some debug attributes, I guess:

error[E0609]: no field `initialized` on type `&mut segment::SegmentBuilder`
   --> webrender/src/segment.rs:212:18
    |
212 |             self.initialized = true;
    |                  ^^^^^^^^^^^
error[E0609]: no field `initialized` on type `&mut segment::SegmentBuilder`
   --> webrender/src/segment.rs:408:28
    |
408 |         debug_assert!(self.initialized);
    |                            ^^^^^^^^^^^
error[E0609]: no field `initialized` on type `&mut segment::SegmentBuilder`
   --> webrender/src/segment.rs:549:18
    |
549 |             self.initialized = false;
    |                  ^^^^^^^^^^^
error: aborting due to 3 previous errors

@mattwoodrow mattwoodrow force-pushed the mattwoodrow:master branch from 69740d5 to f10cd10 Sep 25, 2018
@pcwalton
Copy link
Collaborator

pcwalton commented Sep 25, 2018

Note that there is the arrayvec crate if we want to both allocate on the stack and ensure that a vector never grows beyond a certain capacity.

@gw3583
Copy link
Collaborator

gw3583 commented Sep 25, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2018

📌 Commit f10cd10 has been approved by gw3583

@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2018

Testing commit f10cd10 with merge cc80673...

bors-servo added a commit that referenced this pull request Sep 25, 2018
Improve scene building and frame building performance by reducing allocations

This roughly triples our FPS on the displaylist_mutate talos test, and goes a long way towards resolving #3116.

<!-- 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/3117)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2018

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

@bors-servo bors-servo merged commit f10cd10 into servo:master Sep 25, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 5 files, 3 discussions left (gw3583, mattwoodrow)
Details
Taskcluster (pull_request) TaskGroup: success
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.