Skip to content
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

Should reseting a ByteBlockPool zero out the buffers? #12734

Closed
stefanvodita opened this issue Oct 29, 2023 · 2 comments
Closed

Should reseting a ByteBlockPool zero out the buffers? #12734

stefanvodita opened this issue Oct 29, 2023 · 2 comments

Comments

@stefanvodita
Copy link
Contributor

Description

ByteBlockPool.reset can fill the buffers we're recycling with zeros.

  1. Do we need the buffers to be filled with zeros? Is there some implicit assumption if we were to reuse them that we would only encounter zeros?
  2. If yes, should the responsibility move from ByteBlockPool to the Allocator's recycleByteBlocks method? The ByteBlockPool loses its reference to the buffers it's just zeroed, so it seems outside its concern to zero the buffers. The Allocator could zero the buffers in its recycle method if it needs to.
@stefanvodita
Copy link
Contributor Author

I spent some more time with the code and I can attempt answering the questions in the description.

  1. Yes. We rely on zeros in slice buffers to tell us where a slice ends (the first non-zero byte is the last byte of the slice) (example). There are two cases where we do this. The first is in TermsHashField (code) for a byte pool that is never reset. That means we don't have to zero out the buffers of a byte pool. The second is in MemoryIndex (code) for an int pool that is reset, so we can't skip zeroing out the buffers on reset.
  2. Yes, I think it makes more sense for the Allocator to handle zeroing the buffers, but I don't think there's enough of a benefit to make the change on its own.

So, is there anything to be done? We can remove the option of zeroing byte buffers and move the responsibility for zeroing int buffers to the Allocator. Is that worth doing? Maybe, but it's not obvious to me that there is enough of a benefit to change how things already work.

@stefanvodita
Copy link
Contributor Author

Both the follow-up PRs are merged. I don't think it's worth pursuing this further. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant