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

Wrapping deflated streams in BufferedStream when reading #29

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

Sitterr
Copy link
Contributor

@Sitterr Sitterr commented Mar 14, 2024

Seems that by default ZLibStream and GZipStream don't buffer themselves when decompressing so reading in small chunks(which is the case with nbt tags) is not good for performance
dotnet/runtime#39233

The speed of NbtFile.Read() that I measured:

  • Reading a .mca region file(1024 nbts) combined into 1 large(~6MB) zlib .nbt | zlib
| Method   | Mean       | Error    | StdDev   |
|--------- |-----------:|---------:|---------:|
| Buffered |   296.9 ms |  5.29 ms |  5.19 ms |
| Current  | 1,197.6 ms | 15.89 ms | 14.86 ms |
  • Reading N times the \SharpNBT.Tests\Data\bigtest.nbt | gzip
| Method   | N   | Mean        | Error     | StdDev    |
|--------- |---- |------------:|----------:|----------:|
| Buffered | 1   |    35.08 us |  0.701 us |  1.050 us |
| Current  | 1   |    42.24 us |  0.502 us |  0.470 us |
| Buffered | 100 | 3,374.03 us | 51.648 us | 43.128 us |
| Current  | 100 | 4,238.87 us | 82.226 us | 76.914 us |

@ForeverZer0
Copy link
Owner

I am very much in favor of the concept of this PR, but I notice there is an issue in the implementation that needs to first be corrected: the underlying stream will not get disposed.

Perhaps the constructor of TagReader might be a more suitable place to wrap it, and provide more flexibility if a user is providing their own stream.

Just a rough-draft concept, but something akin to this was my first thought:

  • In TagReader constructor, check if the input stream is already a BufferedStream. If so, nothing needs to be done.
  • If it not a BufferedStream, wrap the stream in one.
  • We must now branch depending on if leaveOpen is true or false.
    • When leaveOpen is true, the only thing TagReader should do is dispose the BufferedStream in its own Dispose method, but leave the underlying stream passed to the constructor alone.
    • When leaveOpen is false, we need some way to track the stream passed to the constructor, as well as the BufferedStream that we created, and dispose of both in TagReader.Dispose.

This is likely going to require the introduction of a new variable to store the stream we wrap, as the BufferedStream would become the BaseStream value, which is stored as an abstract Stream type.

@ForeverZer0
Copy link
Owner

ForeverZer0 commented Mar 15, 2024

I took a look at the source code for BufferedStream, and it appears to call close on the underlying stream,.

It appears to call Close, but not Dispose on the underlying stream. This is probably enough to forget my previous reply and simply merge your PR as-is. The Dispose is a bit superfluous, and will get called by the finalizer anyways, which in turn just ensures that Close was called. Close is what actually frees the resources and releases the handle.

@ForeverZer0 ForeverZer0 merged commit d6e0075 into ForeverZer0:master Mar 15, 2024
1 check passed
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