Support for Memory<T>, Span<T>, ReadOnlySequence<T>, IBufferWriter<T>

[AArnott]

Let's leverage the latest types for high performance.

[AArnott]

@neuecc Expressed some concern about Unity support being compromised if we support this.
Can you elaborate on that? If Unity supports .NET Standard 2.0 now (except for ref-emit support) that sounds like Span<T> should work, since that type is in a nuget package that works on .NET Standard 1.x and 2.0. But as I have almost no experience with Unity I'm eager to learn.

[neuecc]

Unity does not have a mechanism like NuGet's dependency solution.
I think that it is not good to include a Span<T> DLL in MessagePack for C #.

However, it is possible to add support for Span<T> so that Unity will not reference it with # if.
However, adding a dependency on Span<T> to the root level (eg Reader / Writer uses Span )
Both support may become unrealistic.

[AArnott]

Thanks for the heads up. I'm going to experiment with it. I agree, such support goes to the very core of the library and thus would only make sense if it would work on all supported platforms.

I'd very much like to see how perf compares before and after such a change on .NET Framework and .NET Core. If it's comparable, it should theoretically maintain higher throughput because there is even less garbage collection on account of not reallocating and copying ever-growing buffers for large data graphs.

It sounds like there's nothing fundamentally incompatible with Unity... at worst it would just add an extra dependency that folks would have to chain in. At least, so long as .NET Standard / net45 were supported on that Unity platform. It wouldn't work well (if at all) on .NET 2.0 era.

[neuecc]

I just needed to support Span<T> while implementing another library:)

Add Span support to MessagePackBinary.

static class MessagePackBinary
{
#if! UNITY
public int WriteInt32 (Span <T> ...) {}
# endif
}

Also add ref struct MessagePackReader/Writer for long term improvement.
In the first implementation, it will be a wrapper of MessagePackBinary.
However, finally we want to use chunk's linked list for the write buffer.
(Current implementation uses 64K buffer but if overflow, acquire new buffer so it is performance pain point of write large data)

[AArnott]

I'm nearly done with Span<T> support in a branch of the library. So given your interest as well, let's collaborate on this.

[AArnott]

Also add ref struct MessagePackReader/Writer for long term improvement.

Is this so that writer/reader methods don't have to all take the same parameters, but rather can just read from fields?

However, finally we want to use chunk's linked list for the write buffer.
(Current implementation uses 64K buffer but if overflow, acquire new buffer so it is performance pain point of write large data)

This problem I've already solved by accepting IBufferWriter<byte> everywhere instead of ref byte[]. It also eliminates the out int bytesRead parameter in deserializers and int result from serializers for bytes written, so overall writers are far simpler now.

[AArnott]

You can see my fix9 branch to see the changes I've made.

For now, just to get it to work (which is partially does) I've not taken efforts to keep Unity working (and I don't know how to test it anyway). But I think everything I've done can likely work on Unity as well, in theory. And my changes eliminated some redundant code, so it should help keep things simpler going forward. For example, we no longer have dual paths for handling byte[] and Stream. It's always just ReadOnlySpan<byte> for reading and IBufferWriter<byte> for writing. These are compatible with Stream, so it doesn't drop functionality.

[neuecc]

Thanks, amazing design!
Your implementation(IBufferWriter, Sequence, etc...) is great, this result was what I thought as ideal.

I've tried minimum version of your impl in Unity, it works(only in Editor, should check on IL2CPP)

https://gist.github.com/neuecc/a52979d033a89a4d2c648ca548eba68a

This modify has a large breaking change, so if this version mark to 2.*...

• Drop support to Unity .NET 3.5 Runtime Version
• Include System.Buffers, Memory, Runtime.ComplierServices.Unsafe dll in MessagePack-CSharp's distribution package (which is bad manners, but I think that if you can work with Microsoft, you will be allowed to do so)
• Performance on Unity?(IL2CPP not support FastSpan)

It can be the same source code both Core and Unity.

[AArnott]

I've tried minimum version of your impl in Unity, it works(only in Editor, should check on IL2CPP)

Yay! And wow, it's amazing how small a minimal version can be. :)

This modify has a large breaking change, so if this version mark to 2.*...

Yes, I expect this to go into a 2.x version due to it including breaking API changes.

Include System.Buffers, Memory, Runtime.ComplierServices.Unsafe dll in MessagePack-CSharp's distribution package (which is bad manners, but I think that if you can work with Microsoft, you will be allowed to do so)

My employment at Microsoft will help here, only as far as it helps me learn the ins and outs of NuGet and how to workaround its limitations. I won't need their permission to do fancy stuff with the packaging. But yes, my hope is that we can find a way to make Unity consumption convenient while still working well with the other runtimes.

Performance on Unity?(IL2CPP not support FastSpan)

It's true, .NET Core has performance tuning for Span<T> that .NET Framework and the Unity runtime lack, but I believe that's just around offset computation, which we were already doing explicitly before my change. So my hope is that there's no significant perf impact on most runtimes, and perhaps a small perf boost on .NET Core.

I'm down to only 72 test failures out of your 502 unit tests. Working through the rest of the issues may take another day or two, as I have to slot this into my evening time.

[AArnott]

@neuecc Can you check out my fix9 branch and debug into the ComplexToJson test to see what's going wrong? The test is failing with:

System.ArgumentException : LZ4 block is corrupted, or invalid length has been given.

Here are my notes:

master branch (which works)

511 uncompressed bytes
max compressed 529 bytes
compressed buffer size 65809

Encode (with compressed buffer offset 11) -> 206 compressed bytes

LZ4Codec.Encode(byte[65536], 0, 511, byte[65809], 11, 65798) -> 206
LZ4Codec.Decode(byte[217], 11, 206, byte[65809], 0, 511) -> 511

fix9 branch (which fails):

LZ4Codec.Encode(byte[512], 0, 511, byte[1024], 0, 1024) -> 206
LZ4Codec.Decode(byte[1024], 0, 206, byte[512], 0, 512) ->  (throws exception -- 511 is expected result)

What's odd about this is even when the exception is thrown, I can see that the output buffer, trimmed to 511 bytes, correctly has the uncompressed original contents. So why does LZ4 fail?

[neuecc]

If allows to include NuGet dll to unitypackage,
Maybe it can be easy to share definition between .NET Core and Unity.
(Especially it is important to my RPC library, MagicOnion).
I will investigate in detail.

---

I've checked TryDecompress logic, uncompressedSegment.Count is invalid.
uncompressedLength = 511 but uncompressedSegment.Count is 512.
It is come from GetMemory and TryGetArray.

---

By the way, current your branch is too difficult to build.
global.json lock to target .NET Core SDK so if I don't have there verison, I can't build.

(not listed .NET Core 2.x)

I solved delete global.json.

And Nerdbank.GitVersioning shows error,

targets(63,5): error MSB4018: LibGit2Sharp.NotFoundException: object not found - no match for id (7c940a8a3d7c1b5cc4ca186da152548eadb5ed13)

I've comment out all Directory.Build.props lines except LangVersion.

---

I have a question, MessagePackSerializer changes from static to instance.
I can not agree with this.

• MessagePackSerializer.Serialize is better(user friendly) api than new MessagePackSerializer.Serialize.
• MessagePack-CSharp configuration point is IFormatterResolver. If MessagePackSerializer can instantiate, to hold two configuration point, it is complex, lose simpleness.

one more, FromJson should return byte[].
byte[] is easy to use than IBufferWriter, only provides IBufferWriter api is hard to use, not user friendly.

[dzmitry-lahoda]

I have a question, MessagePackSerializer changes from static to instance.
I can not agree with this.

@neuecc , if somebody will create voting issue and more votes will be for instance. Would you agree with that?
I am from 10 years of enterprise outsource and now in gamedev, and could provide (after research of exact code) that instance based ecosystems scale better and eventually are better(user friendlier). Eventually statics are more complex than instance, and solutions on instances are not complex, but necessary engineering to go further.

[neuecc]

@dzmitry-lahoda
No, the fundamentals of the design of the program do not produce good results in voting.
(see Roslyn, dotnet/csharplang)
Resolver chain is the core of MessagePack-CSharp's extensiblity design.
Discussion after understanding correctly is meaningful.
However, if not, it will destroy the design.

[AArnott]

All good points. It is a little more work to use an instance method than a static method, I agree.

But as the serializer is configurable (it used to be configured via a bunch of static fields, which is a non-starter design-wise since it means it can't be used by two consumers with different configuration requirements), we need it to be configurable safely. This can be done either by passing all configuration in as arguments to static methods (which doesn't scale well and undermines the usability you're going for) or it can be on instance fields on the class, which scales much better and conforms to published design guidelines.

Also, since you have multiple serializers (regular, LZ4) having two static classes means that every single use of the serializer has to know whether compression should be used since it has to call the right static method. That's not good. Folks should be able to serialize without knowing what they're serializing to and whether it's compressed. Instance methods allow different serializer instances to be provided to this code so that it's compressed or not based on their caller, making it more loosely coupled (another design guideline).

So my proposal is to keep the primary serializer API as instance methods, and use type derivation wherever possible to allow polymorphism and loose coupling. We can regain the API ease of static methods by also having a static class, configured with reasonable defaults, that also offer basic serialization functionality. Consider Newtonsoft.Json as a case study here: JsonSerializer is an instance class, but the JsonConvert class is also available with static methods and reasonable defaults (and even extra parameters to allow configuration via those static methods). So it's a nice blend of the two worlds.

[AArnott]

I've checked TryDecompress logic, uncompressedSegment.Count is invalid.
uncompressedLength = 511 but uncompressedSegment.Count is 512.
It is come from GetMemory and TryGetArray.

@neuecc Thanks. Passing in 511 instead of 512 indeed fixed the issue. But what's that all about?? I did not expect LZ4Codec to fail if I couldn't predict exactly how large the uncompressed data would end up being. I thought the 3 output buffer parameters I passed to Decode just needed to be at least as big as the uncompressed data would end up being. The Decode method returns the size of the actual uncompressed data, so just like any other read-into-a-buffer function, I thought it would fill up to the buffer I gave it, and tell me how much was actually put in.

Anyway, since you record the uncompressed size when writing the compressed data, I can certainly fix this, but is LZ4 itself actually so limited, or is it just this implementation that somehow has this requirement?

[AArnott]

And Nerdbank.GitVersioning shows error,

The error you mentioned looks like your git database is incomplete. Did you do a shallow fetch/clone? What does git cat-file -p 7c940a8a3d7c1b5cc4ca186da152548eadb5ed13 print out on your machine?

By the way, current your branch is too difficult to build.
global.json lock to target .NET Core SDK so if I don't have there verison, I can't build.

Pinning the .NET Core SDK is the recommended approach, especially if you have Visual Studio Preview installed on your machine, so that the SDK used to build the repo is fixed, without regard to what exact version of VS or latest .NET Core SDK is installed on your machine. It's very good for servicing some release long after you shipped it because the version of the SDK used to build it back then is used to build the servicing branch no matter how old it is. Otherwise, you end up with a servicing branch you can't build a year or two later because the SDK has changed in a breaking way.
It also helps prevent build warnings or other errors that show up on some machines but not others.
Yes, it does mean that developers may have to install a specific SDK version if they don't already have the version pinned by the repo.
You can download the pinned SDK from https://dotnet.microsoft.com/download/dotnet-core/2.1.
If you'd like to pin a newer (stable) version of the .NET SDK, I'm game to update it.

[AArnott]

I have all tests passing!

[AArnott]

@neuecc How do I regenerate Sandbox\Generated.cs? I may need to update the code generator itself as well to adjust to the API changes.

[AArnott]

Never mind. I figured it out. And sent a PR to fix your .tt files: MessagePack-CSharp#370

[neuecc]

@AArnott

it used to be configured via a bunch of static fields

I don't know where are the static fields.
MessagePackSerializer is almost pure function,
itself does not have state and configuration.
And to indicate that it is a pure function, static function is most intuitive.

I also emphasized "maximizing performance by default".
Usually, many people do not cache the serializer instance
so the design to new each time degrade in performance.

Only DefaultResolver is the setting part,
But it is to simplicity for application(should not set by library).
Currently I don't implements CompositeResolver.Create
(because it is slightly difficult to implmentation to use generic type caching)
For that reason, RegisterAndSetAsDefault is often used, but this is not very good.

When creating an extension point as a framework and making the serializer configurable,
the place to use MessagePackSerializer is certainly a little.
Therefore, it is unlikely that it is necessary to pass resolver in many places.

It is a problem that the abstraction related to LZ4 can not be solved.
This can be solved like LZ4StandardResolver.Create(YourCustomResolver)
If I rebuild it with a breaking change, I believe that this way we can keep consistency and simplicity.
(However, this makes it difficult to use the LZ4 serializer (as it requires configuration)
If this is the case, new LZ4MessagePackSerializer will still be more usable.)

Initially I do not think that Newtonsoft.Json's approach is good.
Therefore, MessagePack-CSharp's serializer design is not based entirely on Newtonsoft.Json.
Of course, it is a simple compromise and it will work.

LZ4

LZ4 has frame format and block format.
https://github.com/lz4/lz4/blob/master/doc/lz4_Block_format.md
https://github.com/lz4/lz4/blob/master/doc/lz4_Frame_format.md
MessagePack-CSharp uses block format(with uncompressed length on msgpack ext format) for performance reason.

Git

I've pulled fix9 directly:)

Pinning the .NET Core SDK

Okay, I understand, it is good.
I think it is correct as a reason, but I think it is a bit strict.
Many people (also me) installed only the latest SDK.
A strictly correct system will increase the difficulty of build.
(Actually, I had a hard time finding the cause from the error message)

---

Test passed! Great!
By the way, can you remove Nerdbank.Streams dependency?
Nerdbank.Streams has many unnecessary dependency to use MessagePack serialization,
it makes porting to Unity hard.
Maybe only need Sequence<T>.
Is it okay to include Sequence<T> source code to MessagaPackCSharp repository?

[AArnott]

Only DefaultResolver is the setting part,
But it is to simplicity for application(should not set by library).
Currently I don't implements CompositeResolver.Create
(because it is slightly difficult to implmentation to use generic type caching)
For that reason, RegisterAndSetAsDefault is often used, but this is not very good.

Yes, IIRC it's the DefaultResolver that's the most problematic. It must be removed -- or modified so that no such mutable resolvers are assigned to static fields. Otherwise, the easiest API to use (which omits an explicit resolver) will break when another user of the library changes how the default resolver works. So in effect, the easiest path is also the wrong path for reliability. A good API makes the easiest path and the correct path be the same path.

I also emphasized "maximizing performance by default".
Usually, many people do not cache the serializer instance
so the design to new each time degrade in performance.

You can't make an app's performance better than the app author does for themselves. Even if you allocate 0 memory, you can't make the app fast if the app itself is allocating memory everywhere. Reuse of the serializer can be the app author's responsibility. If they want to reduce GC pressure they can cache and reuse the serializer if they wish. A single, small allocation for the serializer will not hurt them. If they have a very particular scenario where they make many top-level calls into the serializer at once (which seems like an unlikely requirement), and if GC pressure is holding back their perf, then they can certainly cache the serializer at their option. That's a very small fraction of your (potential) user base however, so we should not optimize the API toward their use case. We can enable them to be efficient (which is simply by their caching of their own serializer instance), but only after we make the API easy to use in a safe and flexible way.

When creating an extension point as a framework and making the serializer configurable,
the place to use MessagePackSerializer is certainly a little.
Therefore, it is unlikely that it is necessary to pass resolver in many places.

I agree that best design of an app probably only uses your MessagePackSerializer class in a few places. But even in my early use of it, I found it remarkable that in those places I had to hard-code awareness of whether the result would be compressed or not since you had no instance types that derived from each other, or interfaces. Instead, they were only static types, which can only be used in a small subset of the use cases that instance classes can be used.

Also, you had 6 copies of your serializer's public API: LZ4, !LZ4, LZ4+NonGeneric, !LZ4+NonGeneric, LZ4+Typeless, !LZ4+Typeless. Because these are all static types, they have to be maintained separately (and I found a few bugs in that regard while consolidating as many as I could into one base class), and it means that every user has to call the right static class instead of simply using an instance class that it may be provided by its caller.

Initially I do not think that Newtonsoft.Json's approach is good.
Therefore, MessagePack-CSharp's serializer design is not based entirely on Newtonsoft.Json.
Of course, it is a simple compromise and it will work.

Newtonsoft.Json internally costs a lot in terms of GC pressure, and your MessagePack library has some excellent ways to avoid allocating memory. Some of this took a lot of work (e.g. dynamic code gen, AOT code gen) and I admire your efforts and their results.
What we should strive for is to preserve all the significant work you've done, but borrow the good ideas from other popular libraries and best design practices documented by others to enhance this library. I believe we can preserve the high performance you've worked up while still significantly enhancing the usability and flexibility of this library. #27 makes some progress in this (along with other changes already in my master branch). #30 tracks another API change which we can discuss separately over there.

LZ4 has frame format and block format.

Thanks for that.

Pinning the .NET Core SDK

Okay, I understand, it is good.
I think it is correct as a reason, but I think it is a bit strict.

Yes, I agree it's a bit inconvenient at first. That's why I filed dotnet/sdk#2546 so the .NET Core SDK team can consider improving this.

By the way, can you remove Nerdbank.Streams dependency?
Is it okay to include Sequence source code to MessagaPackCSharp repository?

A reasonable request. However I hesitate to do that because getting Sequence<T> right was really tricky. I had to reverse-engineer how they implemented PipeWriter in corefx since none of the behavior was documented and I had several bugs after I first wrote it that I had to fix. I'm not 100% sure that the current implementation is bug-free either, so copying the source to another project (yours) risks doubling up on the source code that might still contain bugs. So I'd be more comfortable keeping the dependency so that folks can upgrade their Nerdbank.Streams package independently to get whatever fixes they need.
I'll see if Sequence<T> is the only need we have from that library. If so, I can copy the src in and we'll take on the maintenance burden of the copy. I would like to understand the Unity issue more though. We'll still have a couple new assembly dependencies when #27 completes anyway, so as long as we have such dependencies, what will we do to mitigate that on Unity? If we create a bundle of DLLs for Unity folks to consume, does it really make much difference whether it's 2 dll's or 5?

[neuecc]

Reuse of the serializer can be the app author's responsibility

I don't agree with that.
For example, micro benchmark is usually used by default.
Many people will decide the choice of the serializer with reference to the result.
In this kind of library, the fact that the default is fast and less allocation is more meaningful than "actual impact".

Moreover, such many small improvement has a big meaning.
I remember earlier that Scott Hanselman said that performance is not important.
It is a story before .NET Core, your site is not a Stackoverflow.
I hate such remarks.
Now that the .NET Core libraries have made small improvements and improved the performance has a big meaning.
I will not be responsible for this library as it does not allow any small performance degradation and will decline in the default state.

(By the way, new Sequence<T> in Serialize method needs to be retrieved from the pool)

---

Also, since such an API requires a detailed understanding of whether this instance can be cached or not,
Usually I do not think that the serializer can cache it.
I look at code like new DataContraSerailzier().ReadObject.
for example, cache HttpClient instance to static field is the worst experience on .NET.

However, I agree to divide it into an instance and static.
I wanted to abstract LZ4 and Standard, and someone would like to use DI.

In that case, we should give most intuitive api to static api.
In other words, I think that it should not be changed from MessagePackSerializer.Serialize.

---

LZ4

And LZ4 defines streaming api.
https://github.com/lz4/lz4/blob/master/examples/streaming_api_basics.md
Maybe this can use for pipelines.

---

Nerdbank.Streams dependency

Assembly binding in .NET is still complicated and difficult to solve.
Dependence of multiple versions of Newtonsoft.Json has plagued a lot of people.
Even if there is NuGet, less dependence is better.

I understand the concern, but I think copying is better.

In Unity, the size of the library is very important.
In iOS, in order to avoid downloading only Wifi, it must be 150 MB or less (100 MB before).
In Android, can not use apk if over the 100MB(use obb).
Obb causes a lot of troubles, especially it is hard to deal with Unity, so we would like to make it apk if possible.
Therefore, reducing generics with struct(it increase code size), game companies will do various ingenuity (especially Japanese companies, and I too).
It is also important to cut down the code size of just 1MB.

[neuecc]

Is serialize method use in is better?

void Serialize(ref BufferWriter writer, in T value, IFormatterResolver resolver);

But maybe il emit have to rewrite.

[AArnott]

You know it's funny you mention that. I was just thinking about that yesterday as well. That will avoid a copy of potentially large structs. I'll try to incorporate that.