-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-5348: Avoid allocations for Bson*Context #1509
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
Conversation
Hi @obligaron thanks a lot for your PR. I'll take a look, but in the meantime could you rebase on the latest main? We did some changes to the CI and most tests will fail if you don't.
d33f424
to
4f91e8d
Compare
I rebased the PR. Thanks @papafe for looking at the code. 🙂
Hey @obligaron thanks a lot for this PR too.
I understand what you are trying to go but we need to have an internal discussion about how do we want to tackle the issue, so we'll handle this according to our priorities.
Thanks a lot for reviewing the PRs. ❤️
Regarding this one:
The Bson*Context was the top 1 or 2 allocation (depending on the scenario) when profiling our workloads.
We mainly use BsonBinary, so just improving those parts alone would help us a lot.
I could create a new PR with just the BsonBinary(Reader|Writer)Context changes. This would reduce the code changes and hopefully be easier to review.
If this would be a good direction, or if I can help in any other way, please feel free to contact me. 🙂
@obligaron I think this PR makes sense overall, and I understand what you're trying to but we need to have an internal discussion about how we want to implement this, and I'll keep you updated if there are any news.
See CSHARP-5348 for details.
This PR is split into three commits
If the first and second commits should be separated, I can reduce the PR to just the caching logic. For our environment, the
BsonBinaryReader
/BsonBinaryWriter
are also the most important ones.