Skip to content
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

[FEATURE] Add MemoryPack support #89

Closed
jodydonetti opened this issue Nov 22, 2022 · 10 comments
Closed

[FEATURE] Add MemoryPack support #89

jodydonetti opened this issue Nov 22, 2022 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@jodydonetti
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
FusionCache provides a way to add custom serialization format support, by implementing the IFusionCacheSerializer interface.

It already provides 4 implementations:

It would be great to add support for the new super optimized binary format by @neuecc , MemoryPack !

Describe the solution you'd like
A new package that add supports for the MemoryPack format, based on MemoryPack.

Describe alternatives you've considered
Everyone that needs it should otherwise implement their own, which is meh 😐

@jodydonetti
Copy link
Collaborator Author

Just pushed the experimental branch : the code is not that elegant, but this is a first step (and it works).

Thanks @neuecc for your help 🎉

@jodydonetti jodydonetti added the enhancement New feature or request label Nov 22, 2022
@jodydonetti jodydonetti self-assigned this Nov 22, 2022
@jodydonetti jodydonetti pinned this issue Nov 22, 2022
@neuecc
Copy link

neuecc commented Nov 23, 2022

thanks for adding it.

  • Odd project name

CysharpMemoryPack(also NeueccMessagePack) is odd.
In NuGet, MemoryPack, MessagePack is an id unlike Newtonsoft.Json.

  • Require .NET 7 support

Due to the .NET runtime compatibility issue, libraries supporting MemoryPack must be .netstandard2.1; .net7.
Because the codes to be generated are different, #if NET7_0_OR_GREATER, different codes must be included.

  • Register generic formatter

You can use MemoryPackFormatterProvider.RegisterGenericType(Type genericType, Type genericFormatterType).
For example RegisterGenericType(typeof(FusionCacheDistributedEntrySurrogate<>), typeof( FusionCacheDistributedEntryFormatter<>)).

  • about copy and paste formatters

I don't think it's a bad idea, I think it's a legitimate measure.
For example, MessagePack's external extensions (for Unity structs) are just copy and paste.
https://github.com/neuecc/MessagePack-CSharp/blob/master/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/Unity/Formatters.cs

@jodydonetti
Copy link
Collaborator Author

jodydonetti commented Nov 23, 2022

thanks for adding it.

Thanks to you for creating it!

  • Odd project name

CysharpMemoryPack(also NeueccMessagePack) is odd. In NuGet, MemoryPack, MessagePack is an id unlike Newtonsoft.Json.

Yeah it's not a beauty to look at 😅

My thoughts were that, for example, by calling it just "MessagePack" that may have been seen as the MessagePack package, and not a MessagePack package. The thing becomes more obvious for Json, where we have 2 main packages, and also for Protobuf (the one from Marc Gravell and the one from Google).

In this specific case it may make more sense, since the protocol itself comes from that very package.

I'll think about it!

  • Require .NET 7 support

Due to the .NET runtime compatibility issue, libraries supporting MemoryPack must be .netstandard2.1; .net7. Because the codes to be generated are different, #if NET7_0_OR_GREATER, different codes must be included.

Ok so I have to add 2 tfm, got it!

While we are at it: do I also have to let the generator generate the code and grab the code twice?

  • Register generic formatter

You can use MemoryPackFormatterProvider.RegisterGenericType(Type genericType, Type genericFormatterType). For example RegisterGenericType(typeof(FusionCacheDistributedEntrySurrogate<>), typeof( FusionCacheDistributedEntryFormatter<>)).

I missed that, will update it soon.

  • about copy and paste formatters

I don't think it's a bad idea, I think it's a legitimate measure. For example, MessagePack's external extensions (for Unity structs) are just copy and paste. https://github.com/neuecc/MessagePack-CSharp/blob/master/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/Unity/Formatters.cs

Ok, good to know.

Overall thanks again, your help is very welcome 🙏

@neuecc
Copy link

neuecc commented Nov 23, 2022

protobuf-net is Marc's, Google.Protobuf is Google's.
But I think this is judging from the NuGet ID and I would like to follow up as it is..
MessagePack is neuecc/MessagePack, MsgPack.Cli is yfakariya's.
However, I understand what you are saying, and you are right.

While we are at it: do I also have to let the generator generate the code and grab the code twice?

Yes!
Ah...sorry, the main difference, however, is the use of static abstract members.
In the generation of external formatters, even .net7 does not use it, so
Perhaps the same code may be sufficient.

@jodydonetti
Copy link
Collaborator Author

But I think this is judging from the NuGet ID...

Yes, that is something I considered, but in the end I thought it would've been too nuanced for the general public to get right, and preferred being explicit in the name to avoid confusion.

However, I understand what you are saying, and you are right.

Let's hope! I guess we'll find out with the community response.

Also, since I list all the available packages both in the main README and in the cache levels page I hope that may help, too.

Yes! Ah...sorry, the main difference, however, is the use of static abstract members. In the generation of external formatters, even .net7 does not use it, so Perhaps the same code may be sufficient.

Ok got it, I'll keep track of that and may eventually release a new version in case support in .NET will catch up.

Thanks!

@jodydonetti
Copy link
Collaborator Author

Branch updated, now the code feels way more clear!

@neuecc
Copy link

neuecc commented Nov 30, 2022

I've update the document, make wrapper type is easy to create external type formatter.
https://github.com/Cysharp/MemoryPack#serialize-external-types

@jodydonetti
Copy link
Collaborator Author

jodydonetti commented Nov 30, 2022

Woah, thanks @neuecc !

I was thinking about creating an issue to ask for such a feature, but you beat me to it 😬

FYI, just as a thought excercise and a little brainstorming, I thought about the same approach but was worried about the need for extra allocations.
So what I would've asked for would've been something along the lines of this:

public class Foo
{
  public int Prop1 { get; set; }
  public string Prop2 { get; set; }
  public float PropToBeIgnored { get; set; }
}

// IN ANOTHER ASSEMBLY

[MemoryPackableFor(typeof(Foo))]
public partial class FooSurrogate
{
  [MemoryPackInclude]
  public int Prop1 { get; set; }
  [MemoryPackInclude]
  public string Prop2 { get; set; }
  
  [MemoryPackIgnore]
  public float PropToBeIgnored { get; set; }
}

My idea was based on trying to let you change as little as possible in the generator code, with something potentially simple in the first few lines just to change the target type of the generator from the one where the [MemoryPackable] attribute was applied, to the one specified via the new [MemoryPackableFor(...)] attribute. Of course this FooSurrogate class would need to have the same structure as the real Foo class, so we can mark each prop with the attributes that may have been needed (include/ignore/etc), and do a lookup by prop name between the surrogate class and the real class.

Basically the logic would have been something like:

  • declare a surrogate class/struct with the same structure as the one you cannot decorate
  • decorate such surrogate with [MemoryPackableFor(...)] and point to the real type to (de)serialize
  • each prop in the surrogate will be used to know how to treat the corresponding (by name) prop in the real type

I don't know if this approach would have some downsides though, but I wanted to share my idea with you, maybe you can find something interesting.

Anyway I will update the code asap.

Thanks!

@jodydonetti
Copy link
Collaborator Author

Hi, in the end the code was already working well, and the idea of not having to allocate an extra class per each (de)serialization call is nice.

I cleaned up a couple of small things, but apart from that everything remained the same.

I'm almost ready to release it, fingers crossed 🤞

@jodydonetti
Copy link
Collaborator Author

The just released v0.17.0 includes the MemoryPack support 🎉

@jodydonetti jodydonetti unpinned this issue Dec 4, 2022
@jodydonetti jodydonetti changed the title Add MemoryPack support [FEATURE] Add MemoryPack support Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants