-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Make BytesQueueBuffer.get_all
more memory efficient
#3236
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are great, thanks! I would be interested in seeing proof that the get_all
change makes a difference.
result = b"".join(buffer) | ||
buffer.clear() | ||
ret = io.BytesIO() | ||
ret.writelines(buffer.popleft() for _ in range(len(buffer))) | ||
result = ret.getvalue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you able to measure the memory gain? I would expect str.join
to do exactly that under the hood. If it made a difference, the surely we could lower some of the pytest.mark.limit_memory
decorators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With b"".join(buffer)
there was a moment when result
took memory for all the data, but buffer
has not been cleared yet and it took the same amount of memory additionally. So test_memory_usage
reported around 20 MB instead of around 12 MB (5 * 2 MiB chunks + 2 MiB for overhead from the last chunk.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks. I guess I was surprised that no other test showed a memory usage improvement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
@pquentin thanks for your review! |
This PR adds memray tests for functionality added in #3186 and improves memory efficiency of
BytesQueueBuffer.get_all
.