[DisplayList] Remove legacy size fields in DLOp records #56101
Conversation
| uint32_t size : 24; | ||
| explicit DLOp(DisplayListOpType type) : type(type) {} | ||
|
|
||
| const DisplayListOpType type; |
There was a problem hiding this comment.
[Note to reviewers]
The elimination of the size field was the only change really needed in this file, but for some reason deleting that field caused clang-tidy to complain about the type field being uninitialized on every object below. So, I needed to add a parent initializer to every constructor below.
In the end, this is probably a good thing as it allowed me to make the type field const representing its important static role in the identity of the records.
There was a problem hiding this comment.
(But it led to an annoying level of changes to review. Apologies...)
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
Small comment on vector treatment
display_list/dl_builder.cc
Outdated
|
|
||
| storage_.realloc(bytes); | ||
| storage_.trim(); | ||
| offsets_.shrink_to_fit(); |
There was a problem hiding this comment.
I probably would not use shrink_to_fit here since it may imply another O(N) copy.
There was a problem hiding this comment.
Is there a shrink on copy then?
There was a problem hiding this comment.
We've always been trimming the storage, though.
How often is realloc(smaller) another allocation and copy?
There was a problem hiding this comment.
I don't know about realloc, I just mean for the std::vector.
This may cause a reallocation, but has no effect on the vector size and cannot alter its elements.
There was a problem hiding this comment.
With the change to swap mentioned in the other comment, I still trim the storage because we used to do that, but not the offsets since we have no back history of that.
I can eliminate the storage trim in a separate PR so we can track that independently.
I can also investigate if trimming the offsets makes a difference. std::vector seems to use a doubling algorithm which means we could be using twice the storage for the vectors. I'm still not convinced that shrinking it will require an allocation...
There was a problem hiding this comment.
shrinking it may require an allocation, as the reference says. 🤷♂️
display_list/dl_builder.cc
Outdated
|
|
||
| storage_.realloc(bytes); | ||
| storage_.trim(); | ||
| offsets_.shrink_to_fit(); |
There was a problem hiding this comment.
Also if the DLBuilder is meant to be reset I think you may need to swap in a new vector because I don't think std::move is guaranteed to reset it to a neutral state like it does for pointers
There was a problem hiding this comment.
We test reusability. I've been wanting to get rid of re-usability for a while now, but I think there is some code somewhere that relied on that.
There was a problem hiding this comment.
Yeah it would be good to remove it, because then we can have build simply take the memory and leave this object in an invalid state
There was a problem hiding this comment.
I switched to more deliberate uses of std::swap to transfer the data.
There was a problem hiding this comment.
Removing reusability is a job for a separate PR as it will have potential consequences outside of this part of the code.
There was a problem hiding this comment.
(There is already an issue filed for that) flutter/flutter#126589
…157578) flutter/engine@b1c2ba8...c4b0184 2024-10-25 [email protected] [DisplayList] Remove legacy size fields in DLOp records (flutter/engine#56101) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…ne#56101) After landing flutter/engine#54676 we had the DLBuilder using legacy size fields in the DLOp records to manage traversing the records, but we constructed a more direct "offset vector" for the DisplayList to enable random access to the ops. The offset vector was created on the fly during the `Build()` method and then the size fields in the DLOp records were largely unused and redundant. This PR gets rid of the remaining uses of the DLOp size fields and has the Builder produce the offsets vector directly during recording.
2 participants
After landing #54676 we had the DLBuilder using legacy size fields in the DLOp records to manage traversing the records, but we constructed a more direct "offset vector" for the DisplayList to enable random access to the ops. The offset vector was created on the fly during the
Build()method and then the size fields in the DLOp records were largely unused and redundant.This PR gets rid of the remaining uses of the DLOp size fields and has the Builder produce the offsets vector directly during recording.