Closed Sitterr closed 4 months ago
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:
TagReader
constructor, check if the input stream is already a BufferedStream
. If so, nothing needs to be done.BufferedStream
, wrap the stream in one.leaveOpen
is true
or false
.
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.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.
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.
Seems that by default
ZLibStream
andGZipStream
don't buffer themselves when decompressing so reading in small chunks(which is the case with nbt tags) is not good for performance https://github.com/dotnet/runtime/issues/39233The speed of
NbtFile.Read()
that I measured:Reading a .mca region file(1024 nbts) combined into 1 large(~6MB) zlib .nbt | zlib
Reading N times the
\SharpNBT.Tests\Data\bigtest.nbt
| gzip