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

CSHARP-5348: Avoid allocations for Bson*Context #1509

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

obligaron
Copy link
Contributor

See CSHARP-5348 for details.

This PR is split into three commits

  • First, adds PopContext if it is missing from Bson*Context (no logic change)
  • Second, adds a PushContext method (no logic change)
  • The third adds the caching logic to Bson*Context.

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.

@obligaron obligaron requested a review from a team as a code owner October 21, 2024 20:32
@obligaron obligaron requested review from adelinowona and removed request for a team October 21, 2024 20:32
@papafe papafe self-requested a review November 22, 2024 11:12
@papafe
Copy link
Contributor

papafe commented Nov 22, 2024

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.

@obligaron
Copy link
Contributor Author

I rebased the PR. Thanks @papafe for looking at the code. 🙂

@adelinowona adelinowona removed their request for review November 22, 2024 18:46
@papafe
Copy link
Contributor

papafe commented Nov 26, 2024

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.

@obligaron
Copy link
Contributor Author

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. 🙂

@papafe
Copy link
Contributor

papafe commented Nov 27, 2024

@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.

@papafe papafe removed their request for review December 18, 2024 16:00
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.

2 participants