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

Use custom struct BufferWriter instaed of StringBuilder in IonReader #19

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

lechu445
Copy link
Contributor

@lechu445 lechu445 commented Feb 1, 2021

Optimization with better separation of concerns.

Main goal of this optimisation is to remove usage of StringBuilder used as intermediate buffer for line. The StringBuilder object was allocated and it allocated internal buffer for string. I changed it with light struct that writes data directly into rented char array that is exposed in CurrentRawLine. It also improves separation of concerns in IonReader.

The other changes:

  • used sealed keyword where possible to increase probability of JIT optimizations
  • changed names of some variables to more meaningful
  • added attribute [DoesNotReturn] to ThrowHelper methods to improve nullable code analysis
  • made methods static where possible

@wmaryszczak wmaryszczak merged commit 7ef9ab3 into anixe:master Feb 1, 2021

namespace Anixe.Ion.Helpers
{
internal struct BufferWriter : IDisposable
Copy link
Contributor

Choose a reason for hiding this comment

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

pls, add method ToString() and add the struct into Anixe.IO, we can reuse it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BufferWriter is implemented specially for Anixe.Ion and does not handle unnecessary overhead, e.g. input validation. That's why it is internal. And incorrect usage can lead to memory leaks, e.g. if somebody will pass the struct as method parameter, JIT will create defensive copy of the struct, and both will contain the same buffer reference.

I guess you think about use case to use it for no-allocation string building.
In my opinion, better approach is to implement ValueStringBuilder used internally in .NET Core library. It uses the same idea as BufferWriter but also for short strings it uses stackallock`ated buffer.
I remember we had discussion about ValueStringBuilder in our system. The answer was to wait for it public in .NET Core library. Currently it is still an open issue in dotnet/runtime#25587. They are waiting for language support protect of incorrect usage of this struct when accidental defensive copies are made. They need compiler feature that requires struct be passed only by reference and not by value.

I propose implement ValueStringBuilder in Anixe.IO and put a documentation comment that the struct MUST be passed by reference. I hope, the developers at ANIXE read comments ;)

@wmaryszczak What do you think?

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