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

Add PrimitiveScratchBuffer implementation. #3335

Merged
merged 3 commits into from Nov 22, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Collaborator

gw3583 commented Nov 21, 2018

This splits some information from the PrimitiveStore into the new
scratch buffer type.

The scratch buffer is specifically for storing information that
is only read / written during frame building, as opposed to other
items in the primitive store which is created during scene building.

Since the scratch buffer information is not moved between threads,
it's easy to retain it between both frame builds and also when a
new display list is sent by the scene builder thread.

When a new scene arrives, the recycle method allows the storage
of the scratch buffer to possibly be shrunk, if it is now
consuming much more memory than was used on the previous frame.

NOTE: This initial commit only uses the scratch buffer for clip
mask instances, but there will be more fields moved to make
use of the retained scratch buffer soon.


This change is Reviewable

Add PrimitiveScratchBuffer implementation.
This splits some information from the PrimitiveStore into the new
scratch buffer type.

The scratch buffer is specifically for storing information that
is only read / written during frame building, as opposed to other
items in the primitive store which is created during scene building.

Since the scratch buffer information is not moved between threads,
it's easy to retain it between both frame builds and also when a
new display list is sent by the scene builder thread.

When a new scene arrives, the recycle method allows the storage
of the scratch buffer to possibly be shrunk, if it is now
consuming much more memory than was used on the previous frame.

NOTE: This initial commit only uses the scratch buffer for clip
      mask instances, but there will be more fields moved to make
      use of the retained scratch buffer soon.
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 21, 2018

r? @nical or @kvark or anyone

I don't expect this to have measurable effect on dl_mutate and friends, but it should help in the future as more fields make use of it.

Pending try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed36d570f1bf761d4dd589fc23694582610bc6f2

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 22, 2018

Added a follow up commit that moves the glyphs array to the retained scratch buffer.

Initialize prim store vecs with estimated capacity need.
Record the used size of the prim store vecs that can't be retained
after each scene build, and use this as a capacity estimate for
those arrays next time we build a scene.
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 22, 2018

Added a third commit:

Record the used size of the prim store vecs that can't be retained
after each scene build, and use this as a capacity estimate for
those arrays next time we build a scene.

Pending try with all those patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=295fa451a53f7b4480eedc192906d4cd9285cd5e

@kvark

kvark approved these changes Nov 22, 2018

:lgtm:

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


webrender/src/frame_builder.rs, line 18 at r3 (raw file):

use prim_store::{PrimitiveStore, SpaceMapper, PictureIndex, PrimitiveDebugId, PrimitiveScratchBuffer};
#[cfg(feature = "replay")]
use prim_store::{PrimitiveStoreStats};

why do we need the stats for capture replay?


webrender/src/util.rs, line 784 at r3 (raw file):

        // a frame with exceptionally large allocations to cause subsequent frames to retain
        // more memory than they need.
        vec.shrink_to_fit();

shouldn't we leave some headroom here? if we shrink to fit, it's likely to need to grow afterwards

@gw3583

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kvark)


webrender/src/frame_builder.rs, line 18 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why do we need the stats for capture replay?

An empty stats object is passed on loading a capture.


webrender/src/util.rs, line 784 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

shouldn't we leave some headroom here? if we shrink to fit, it's likely to need to grow afterwards

I think it's probably ok - since we only shrink to fit if the used size if < 0.5 the capacity. But we can do some tuning / profiling here of how often it shrinks.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 22, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 22, 2018

📌 Commit 8d1a776 has been approved by kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 22, 2018

⌛️ Testing commit 8d1a776 with merge 3d7a8fa...

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

Auto merge of #3335 - gw3583:recycle-storage, r=kvark
Add PrimitiveScratchBuffer implementation.

This splits some information from the PrimitiveStore into the new
scratch buffer type.

The scratch buffer is specifically for storing information that
is only read / written during frame building, as opposed to other
items in the primitive store which is created during scene building.

Since the scratch buffer information is not moved between threads,
it's easy to retain it between both frame builds and also when a
new display list is sent by the scene builder thread.

When a new scene arrives, the recycle method allows the storage
of the scratch buffer to possibly be shrunk, if it is now
consuming much more memory than was used on the previous frame.

NOTE: This initial commit only uses the scratch buffer for clip
      mask instances, but there will be more fields moved to make
      use of the retained scratch buffer soon.

<!-- 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/3335)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 22, 2018

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

@bors-servo bors-servo merged commit 8d1a776 into servo:master Nov 22, 2018

3 of 4 checks passed

code-review/reviewable 2 discussions left (kvark)
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