search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.39k stars 4.75k forks source link

Investigate switching to ArrayBufferWriter impl to simplify grow logic when reading from stream in JsonSerializer #32355

Open ahsonkhan opened 4 years ago

ahsonkhan commented 4 years ago

https://github.com/dotnet/runtime/blob/d4b06b1a9e3c56e343c42efad2211c04c196a0cf/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs#L95

cc @steveharter

davidfowl commented 4 years ago

ArrayBufferWriter doesn't pool currently. That's likely a performance regression

ahsonkhan commented 4 years ago

Yep, agreed. Since this is used internally, in this case, we can consider using PooledByteBufferWriter just like what we do on the Serialize/Write side.

scalablecory commented 4 years ago

Consider using the ArrayBuffer we use in System.Net.

ahsonkhan commented 4 years ago

That's essentially identical to PooledByteBufferWriter, if it also implemented the IBufferWriter<byte> interface (with slightly different growth logic). We should consider merging these and having only one copy.

https://github.com/dotnet/runtime/blob/f06f46c049e442d34950884783b4b85f7e6c1532/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PooledByteBufferWriter.cs#L13-L16

ghost commented 3 years ago

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of the experimental issue cleanup initiative we are currently trialing in a limited number of areas. Please share any feedback you might have in the linked issue.

ahsonkhan commented 3 years ago

The source still has the todo comment, likely worth addressing manually, or closing explicitly.