Repository navigation
Conversation
github-actions
Bot
added
engine
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
a3d3cf9 to
9b81dd9
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates HostBuffer in the Flutter GPU library to reuse previously allocated blocks across frames when crossing block boundaries, and corrects the lengthInBytes of the returned BufferView to match the actual written bytes. Unit tests were added to verify these changes. Feedback suggests simplifying the block retrieval logic in buffer.dart by extracting the frame buffers list into a local variable and refactoring the conditional check to improve readability.
| final DeviceBuffer buffer; | ||
| if (_bufferCursor < _buffers[_frameCursor].length) { | ||
| buffer = _buffers[_frameCursor][_bufferCursor]; | ||
| } else { | ||
| buffer = _allocateNewBlock(blockLengthInBytes); | ||
| _buffers[_frameCursor].add(buffer); | ||
| } |
There was a problem hiding this comment.
This block can be simplified to improve readability and maintainability. By extracting _buffers[_frameCursor] into a local variable, we can avoid repeated list lookups. Additionally, we can eliminate the uninitialized final DeviceBuffer buffer; declaration and the else block by checking if we need to append a new block first, and then unconditionally retrieving the buffer at _bufferCursor.
final List<DeviceBuffer> frameBuffers = _buffers[_frameCursor];
if (_bufferCursor >= frameBuffers.length) {
frameBuffers.add(_allocateNewBlock(blockLengthInBytes));
}
final DeviceBuffer buffer = frameBuffers[_bufferCursor];References
- Suggest simplification and refactoring: Assess whether the code can be made simpler or refactored to enhance readability and maintainability. (link)
The block-rollover branch of _allocateEmplacement always allocated a new block and appended it, without checking whether the frame slot already held one at that cursor from an earlier frame. reset() only rewinds the cursors, so every frame whose transient writes crossed a boundary appended another block, for the life of the HostBuffer. The same branch also returned a BufferView describing the whole block rather than the write it had just placed.
9b81dd9 to
4266af5
Compare
dzolotov
changed the title
HostBuffer._allocateEmplacement's block-rollover branch always allocated a new block and appended it, without checking whether the frame slot already held one at that cursor from an earlier frame.reset()only rewinds the cursors, so every frame whose transient writes crossed a block boundary appended anotherblockLengthInBytesblock, for the life of theHostBuffer. The same branch also returned aBufferViewdescribing the whole block rather than the write it had just placed.Fixes #192392
The off-by-one in the size check itself is left alone here — that is #186526, with PR #186539 already open for it.
The two new tests need a block boundary, so they create a
HostBufferwith a two-alignment block rather than the 1 MB default. The existingHostBuffer reuses DeviceBuffers after N framestest only ever exercises the first block, which is allocated in the constructor, and that is why the missing reuse of later blocks went unnoticed.Verified locally on macOS/Metal,
host_debug_unopt_arm64.gpu_test.dartrun underflutter_tester --enable-impeller --impeller-backend=metal --enable-flutter-gpupasses: 99 passed, 1 skipped. Reverting only thebuffer.dartchange and rebuilding makes exactly the two new tests fail, which is what they are there for:The reproduction from the issue, run on macOS/Metal against a 3.47.0 SDK with the same patch applied:
DeviceBuffers over 200 crossingsframeCount)BufferView.lengthInBytesfor a 256-byte write(An earlier version of this description also quoted process RSS. I've dropped it: the reproduction allocates 200 MB of
ByteDataof its own, so RSS varies by ~120 MB between runs of the same binary and says nothing useful here. The block count is the stable measurement.)Pre-launch Checklist
///).