Repository navigation
renderer/metal: fix 2x sizeof over-allocation in Buffer.sync - #13490
Merged
Merged
Conversation
Buffer.sync and Buffer.syncFromArrayLists computed the new buffer size in bytes (req_bytes * 2) and then multiplied by @sizeof(T) again when passing it to newBufferWithLength:, allocating data.len * sizeOf(T)^2 * 2 bytes. For the 32-byte CellText buffers this is a 64x over-allocation and for the 4-byte CellBg buffers 8x, per swap-chain frame (e.g. ~9.4MB instead of ~300KB per frame for a full 120x40 screen of text). Match the OpenGL buffer implementation: track the new length in units of T and multiply by @sizeof(T) exactly once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seems to me the grow path in
Buffer.sync(andsyncFromArrayLists) multiplies by@sizeOf(T)twice:req_bytesis already a byte count, it gets doubled into size, and then thenewBufferWithLength:call doessize * @sizeOf(T)on top of that. So every reallocation ends up beingdata.len×@sizeOf(T)^2 × 2 bytes instead of the intendeddata.len×@sizeOf(T)× 2.The OpenGL version of this same helper does it what seems to be the intended way, so this looks like a mixup rather than a deliberate safety margin.
This makes the Metal implementation match the OpenGL one: track the new length in units of T (which also fixes self.len going stale after a grow. It's documented as the allocated element count but was never updated here)