Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRecycle some of the vector allocations when re-building the frame. #1158
Conversation
|
This helps a bit (~12% improvement) with issue #1031 and is relevant to issue #1029, although the latter advertises using sensible defaults for vector allocations which is a different/complementary approach that could typically help with the first frame. After this there are still outstanding reallocations when profiling issue #1031. I'll follow up with those. r? @kvark |
| @@ -320,6 +320,10 @@ impl Frame { | |||
| background_color, | |||
| self.frame_builder_config); | |||
|
|
|||
| if let Some(ref mut old_builder) = self.frame_builder { | |||
This comment has been minimized.
This comment has been minimized.
kvark
Apr 24, 2017
Member
I think it would be cleaner to do something like
let mut frame_builder = self.frame_builder.take().unwrap_or(FrameBuilder::new(...));
frame_builder.clear(); // resets all the contents, keeps the capacity/allocation
This comment has been minimized.
This comment has been minimized.
nical
Apr 24, 2017
Author
Collaborator
I started with something like this and the reason I ended up changing is that if you add members to FrameBuilder it is easy to forget to handle them in clear() which can lead to some subtle misbehavior. With this approach, if you forget to handle the member in recycle you won't have a bug (just potentially miss an optim, which will show up in profiles if it turns out to have any impact).
If you have a strong preference for doing it the other way I can rearrange the patch.
This comment has been minimized.
This comment has been minimized.
kvark
Apr 24, 2017
Member
I see. You have a point. I wish we could do #[derive(Clear)] to enforce that without manually tracking the new items.
This comment has been minimized.
This comment has been minimized.
kvark
Apr 24, 2017
Member
Another approach that could be cleaner is to pass self.frame_builder as a parameter into FrameBuilder::new, which would just take & clear the vectors it needs from the old one. That would make sure no items are left behind (without recycle_vec, which feels a bit hacky to me).
|
@bors-servo r+ |
|
|
Recycle some of the vector allocations when re-building the frame. Currently, most of the data in FrameBuilder is regenerated from scratch when RenderBackend::build_scene is called. When this happens all of the FrameBuilder data from the previous frame still exists but is thrown away as we re-allocate a new FrameBuilder. In some test cases a lot of time is spent growing vectors when building frames. We can take advantage of the likelihood that the next frame will require the same amount of allocations as the previous frame and reduce the cose of reallocating by recycling vectors from the previous frame. This commit does that for some of the vectors in FrameBuilder and PrimitiveStore which show up high in some profiles. This commit does not reuse an allocation if it is more than twice the size of the actually used data, so that if a frame allocates an outstanding amount of memory, the peak does not stay allocated forever. <!-- 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/1158) <!-- Reviewable:end -->
|
|
nical commentedApr 24, 2017
•
edited by larsbergstrom
Currently, most of the data in FrameBuilder is regenerated from scratch when RenderBackend::build_scene is called. When this happens all of the FrameBuilder data from the previous frame still exists but is thrown away as we re-allocate a new FrameBuilder. In some test cases a lot of time is spent growing vectors when building frames. We can take advantage of the likelihood that the next frame will require the same amount of allocations as the previous frame and reduce the cose of reallocating by recycling vectors from the previous frame. This commit does that for some of the vectors in FrameBuilder and PrimitiveStore which show up high in some profiles.
This commit does not reuse an allocation if it is more than twice the size of the actually used data, so that if a frame allocates an outstanding amount of memory, the peak does not stay allocated forever.
This change is