Skip to content

Implementing ScratchBufferAllocator #1247

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

Merged
merged 21 commits into from
Jun 27, 2025
Merged

Implementing ScratchBufferAllocator #1247

merged 21 commits into from
Jun 27, 2025

Conversation

TalZaccai
Copy link
Contributor

@TalZaccai TalZaccai commented Jun 11, 2025

ScratchBufferAllocator is similar to ScratchBufferBuilder (previously ScratchBufferManager), but instead of creating a new buffer and copying the previous data when the buffer has no sufficient space (and releasing the pointer to the previous buffer), it instead keeps a stack of the previous buffers and does not copy the previous data.
By doing so, ScratchBufferAllocator ensures that any previously created ArgSlices that have not been explicitly re-winded do not point to GCed memory.
The reason that both managers are currently necessary, is that ScratchBufferBuilder supports getting all the data in a continuous chunk of memory, where ScratchBufferAllocator does not.

@TalZaccai TalZaccai marked this pull request as ready for review June 11, 2025 05:37
@Copilot Copilot AI review requested due to automatic review settings June 11, 2025 05:37
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements the new ScratchAllocationManager to manage memory allocations without copying existing data and updates all relevant usages from ScratchBufferManager to ScratchAllocationManager.

  • Implement new ScratchAllocationManager with backup buffer reuse
  • Update transaction, response, and custom command code to use ScratchAllocationManager
  • Adjust unit tests to validate behavior of the new manager

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/Garnet.test/ScratchAllocationManagerTests.cs Added tests to verify correct allocation, rewind, and reset behavior
libs/server/Transaction/TxnRespCommands.cs Updated custom procedure invocation to use ScratchAllocationManager
libs/server/Transaction/TransactionManager.cs Replaced scratch buffer manager with scratch allocation manager
libs/server/Resp/RespServerSession.cs Initialized and reset the new ScratchAllocationManager alongside the old manager
libs/server/Custom/CustomTransactionProcedure.cs Updated method calls to use ScratchAllocationManager
libs/server/Custom/CustomRespCommands.cs Updated call to GetCustomTransactionProcedure to use the allocation manager
libs/server/Custom/CustomCommandManagerSession.cs Revised method signature and assignment for scratch allocation manager
libs/server/ArgSlice/ScratchAllocationManager.cs New implementation of ScratchAllocationManager with expanded buffer logic
libs/common/RefStack.cs Updated to use new C# 12 array initialization for an empty stack

@TalZaccai TalZaccai requested review from badrishc and TedHartMS June 11, 2025 05:37
@TalZaccai TalZaccai changed the title Implementing ScratchAllocationManager Implementing ScratchBufferAllocator Jun 13, 2025
@TalZaccai TalZaccai requested a review from badrishc June 13, 2025 01:08
@TalZaccai TalZaccai merged commit aa92fb7 into main Jun 27, 2025
46 of 48 checks passed
@TalZaccai TalZaccai deleted the talzacc/sbm branch June 27, 2025 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants