Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #468 +/- ##
==========================================
+ Coverage 71.42% 71.97% +0.55%
==========================================
Files 353 358 +5
Lines 14332 14425 +93
Branches 1209 1213 +4
==========================================
+ Hits 10237 10383 +146
+ Misses 3698 3637 -61
- Partials 397 405 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JR-Morgan
force-pushed
the
jrm/stj-experiment
branch
from
4558f3e to
72f6898
Compare
JR-Morgan
force-pushed
the
jrm/stj-experiment
branch
from
ea0a9ed to
87dc3ac
Compare
| /// <summary> | ||
| /// Represents a heap-based, array-backed output sink into which <typeparam name="T"/> data can be written. | ||
| /// </summary> | ||
| public sealed class ArrayBufferWriter<T> : IBufferWriter<T> |
There was a problem hiding this comment.
Like how before, we were pooling StringBuilders as the buffer behind the newtonsoft json writer.
The STJ equivalent is these ArrayBufferWriter
We're pooling them in the same way
| { | ||
| using var fileStream = new FileStream(tempFilePath, FileMode.Create, FileAccess.Write, FileShare.None); | ||
| using var gzip = new GZipStream(fileStream, CompressionLevel.Optimal); | ||
| using var writer = new StreamWriter(gzip); |
There was a problem hiding this comment.
Since the ArrayBufferWriter<byte> is buffering bytes not chars, we don't need to use a StreamWriter anymore, we can directly write to the gzip stream
| using var jsonWriter = new JsonTextWriter(stringWriter); | ||
| using var idWriter = new SerializerIdWriter(jsonWriter); | ||
| var efficientJson = new EfficientJson(); | ||
| using var jsonWriter = new Utf8JsonWriter(efficientJson.Buffer); |
There was a problem hiding this comment.
This Utf8JsonWriter is the "start" of our journy into STJ
Utf8JsonWriters are backed by an ArrayBufferWriter (which we're pooling in the similar way we were previously pooling string buildres)
| #if NET6_0_OR_GREATER | ||
| string id = IdGenerator.ComputeId(efficientJson.WrittenSpan); | ||
| #else | ||
| string id = IdGenerator.ComputeId(efficientJson.GetInternalBuffer(), 0, efficientJson.WrittenCount); |
There was a problem hiding this comment.
Since the ArrayBufferWriter gives us "low level" access to its buffer, we can avoid having a seprate id writer.
| // Will throw JsonWriterException if not supported | ||
| writer.WriteValue(value); | ||
| return; | ||
| throw new ArgumentOutOfRangeException(nameof(value), $"Unsupported type {value.GetType()}"); |
There was a problem hiding this comment.
Newtonsoft gave a handy WriteValue(object) overload which supported a bunch of primitives, and would throw if not supported.
STJ doesn't have an equivalent, so we've added cases for all the primitives higher up in this switch statement. Then this will throw like before if not supported.
1 participant
This is a semi-experimental change to replace newtonsoft with System.Text.Json for the new send pipeline serializer
Newtonsoft has been largely stale for the past 5 years; and System.Text.Json has exceeded it's performance considerably. For us, this mostly comes down being able to utilize modern .NET features to reduce memory allocations.
Historically, we've have been hesitant to adopt STJ because
But for the following reasons, I think we're now in a good position to consider the switch
Serializer Benchmarks - https://app.speckle.systems/projects/bf5b49215c/models/0d573b9a34
NET 10
NET 8.0
NET 4.8
TODO:
I'm expecting the .NET8 and .NET 10 targets to be most benefited.✅Memory allocations should be measurably lower.✅Raw speed should also be improved too✅.netstandard2.0 less so (I'm not expecting any regressions, but we should test)✅Ensure test coverage of the simple stuff (id generation etc...)✅IL repack System.Memory? or no?❎Can't since we need to expose theIBufferWriterinterface from Speckle.Sdk.Depedencies