Closed Bug 1405445 Opened 2 years ago Closed 2 years ago

Preallocate the display list buffer to a reasonable size.

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

(Whiteboard: [wr-mvp])

Attachments

(1 file)

No description provided.
Assignee: nobody → jmuizelaar
Attachment #8914873 - Flags: review?(bugmail)
Comment on attachment 8914873 [details] [diff] [review]
Preallocate the display list buffer to a reasonable size

Review of attachment 8914873 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/webrender_bindings/src/bindings.rs
@@ +1077,3 @@
>          WebRenderFrameBuilder {
>              root_pipeline_id: root_pipeline_id,
> +            dl_builder: webrender_api::DisplayListBuilder::with_capacity(root_pipeline_id, content_size, capacity),

I'd slightly prefer if we used the old ::new function instead of ::with_capacity in the case where capacity==0 here. It seems not so great to explicitly initialize the vec with zero capacity for the cases where we pass 0, because those cases really mean "we don't know what size to use", not "we will actually never put anything in here". If we start with zero capacity for the former cases it seems like we're going to do extra reallocations when we could just start with a small-but-nonzero capacity and save some reallocs.

Of course this change would only help if we modified the upstream DisplayListBuilder::new impl to not pass 0 to with_capacity, but instead pass some small-but-nonzero default value.

I don't feel strongly about this, leaving it to you.
Attachment #8914873 - Flags: review?(bugmail) → review+
Also, if you're going to land this soonish, please land it on autoland because otherwise it will conflict with my layers-full deletion patches.
By convention ::new() is expected to behave ~identically to ::with_capacity(0). I think if we want to do minimum sizing we should handle that in the client, since gecko/servo don't necessarily agree on the issue.
Whiteboard: [wr-mvp] [triage]
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0376b0f2075a
Preallocate the display list buffer to a reasonable size. r=kats
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
This conflicted with my merge to m-c (which included changes from bug 1391816, touching a lot of similar files), so I backed this out to get things merged. Feel free to rebase and reland whenever.

Backout: https://hg.mozilla.org/mozilla-central/rev/086a9a00753e466752c14a255c1ab2a9ddb25b15
Flags: needinfo?(jmuizelaar)
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08cf5146eff1
Preallocate the display list buffer to a reasonable size. r=kats
https://hg.mozilla.org/mozilla-central/rev/08cf5146eff1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.