-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add a reference to MessagePack implementation #14
Conversation
It's very rough and limited. And it assumes eiriktsarpalis#12 is addressed.
Incidentally, upgrading the baseline from MessagePack v2 to v3 produces this: Serialization
This improves serialization slightly, making TypeShape fair slightly worse by comparison. Deserialization
Deserialization improves significantly, making TypeShape look quite a bit worse. This is due to using a dictionary to look up property names, which the AOT formatter avoids. |
I suspect it's because in this version, we call |
private delegate T? FormatDeserialize<T>(ref MessagePackReader reader, MessagePackSerializerOptions options); | ||
private delegate void FormatSerialize<T>(ref MessagePackWriter writer, T? value, MessagePackSerializerOptions options); | ||
|
||
private sealed class Formatter<T>(FormatDeserialize<T> deserialize, FormatSerialize<T> serialize) : IMessagePackFormatter<T?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why you went for a delegate adapter, but it does result in two layers of indirection for every call, which adds up as the object graph is being traversed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Can you review all the places where formatters.GetOrAdd
appears in my code and tell me if I'm doing it right? I don't understand why this pattern is required, so I'm not sure...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting that you could make Formatter<T>
an abstract class and have individual types and shapes inherit from that. The downside is that you need to factor those into individual classes as opposed to inlining lambda expressions, but on the other hand splitting your code into a folder of Formatters and then a keeping a small visitor that just folds them together should make things more manageable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, I'm not sure what I'd do with more than the two implementations I have.
Maybe I'll need more as I get to supporting non-default constructors, etc. though.
}; | ||
|
||
internal IMessagePackFormatter<T?> GetFormatter<T>(ITypeShape<T> typeShape) | ||
{ | ||
return formatters.GetOrAdd<IMessagePackFormatter<T?>>(typeShape, this, box => new Formatter<T?>(box.Result.Deserialize, box.Result.Serialize)); | ||
return formatters.GetOrAdd<IMessagePackFormatter<T?>>(typeShape, this, box => box.Result switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delayed value factory is a late binding mechanism used when a recursive type is encountered. The idea is that it's creating a wrapper for box.Result
which will eventually contain a reference to the final computed value once building has completed.
For this to work, evaluating box.Result
needs to be delayed until an actual serialization operation is run. Here's how it's done for CBOR:
And here's how the object cloner does it:
@neuecc this should interest you. |
…till deserialization
@@ -592,15 +592,27 @@ public StructWithDefaultCtor() | |||
} | |||
|
|||
[GenerateShape] | |||
public partial class BaseClass | |||
public partial class BaseClass : IEquatable<BaseClass> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to factor the IEquatable implementation to a separate set of types so that we coverage for both cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this because I had been testing round-tripping. I'm not sure what case we're not covering when this interface isn't present.
But I did just discover IsEquatable
on test cases, which probably was false
for this type up to this point and why no one else hit the fact that this class didn't know how to do a by-value compare of itself.
I can remove this change if you'd like. I don't need it any more.
This pull request includes updates to the
README.md
file, enhancements to theSpanDictionary
class, and improvements to the equality members of theBaseClass
andDerivedClass
in the test types.Documentation Updates:
README.md
: Added a new section listing known libraries based on TypeShape, includingNerdbank.MessagePack
.Code Enhancements:
src/TypeShape.Examples/Utilities/SpanDictionary.cs
: Added a new overload for theToSpanDictionary
method to support mapping with a value selector.Test Improvements:
tests/TypeShape.Tests/TestTypes.cs
: ImplementedIEquatable<T>
forBaseClass
andDerivedClass
, and added appropriateEquals
andGetHashCode
methods.