by

Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
    Yoshifumi Kawai
    @neuecc
    @AArnott sorry, is this final chance of breaking change? to implementing new lz4, I've noticed should change bool UseLZ4Comparresion to enum. #675
    Yoshifumi Kawai
    @neuecc
    thanks for the hard work in last few days.
    completely my concern is solved.
    wait the release:)
    Andrew Arnott
    @AArnott
    Yay. :)
    I'm happy with what we have now too. Since I don't need it stable this week, and we had some changes this past week, I suggest we give it another week of use to "stabilize" before removing the -rc and pushing the package.
    It's been great working with you, @neuecc
    Yoshifumi Kawai
    @neuecc

    Thanks, I’ve been happy too.

    my ex-colleague, clients are trying preview.
    I've been pointed one immediately. #698 :)

    Andrew Arnott
    @AArnott
    @neuecc What do you think of removing the MessagePackSerializer.Deserialize(Stream) methods in v2.0? We can support that scenario much better with #388 using a class that can track state between reads, thus enabling reading a message at a time from the stream but in a highly performant way. I think the way MessagePackSerializer.Deserialize(Stream) works today is unintuitive, and fixing it later to allow reading a message at a time wouldn't work because we need to store state between each read. I could even do it for 2.0 if necessary, but we'd need to agree on the removal of the method first.
    Yoshifumi Kawai
    @neuecc
    @AArnott Deserialize (Stream) is a standard method, so if it is deleted, it will be quite difficult to use as an API.
    for example, many people will expect the following API (sample from AspNetCoreMvcFormatter),
    MessagePackSerializer.DeserializeAsync (request.Body)
    Also, the processing of Buffer by Sequence is included,
    It is more efficient than user created buffer.
    Of course, I can understand the disadvantages (which causes unexpected behavior), but as a trade-off.
    What about APIs that provide length hints instead?
    For example, AspNetCoreMvcFormatter
    We can write DeserializeAsync (context.ModelType, request.Body, request.ContentLength)
    This is certainly useful for streams with a length prefix.
    Andrew Arnott
    @AArnott
    Yes, discoverability of the right API is a concern. and having a Serialize(Stream) method but no Deserialize(Stream) method feels odd.
    In your case of providing lengths, you're providing the length of the Body stream anyway, so that's no better than if we just read to the end of the stream. But yes, in general if folks know the length of a slice of the stream to read from, they should be able to pass that in if we depended on it.
    Nerdbank.Streams offers a Stream.Slice extension method that already restricts reading to a given length. But having it built in to messagepack might be useful for folks who don't have that.
    On the other hand, I wonder if a stream has msgpack in it, wouldn't it either end with that msgpack structure or have more msgpack structures? So if we offered an API that can repeatedly read from a stream such that the length in advance doesn't have to be known, that sounds like what folks expect based on the several issues that folks are asking for this on.
    Thinking more about this, how about a MessagePackStreamReader and MessagePackStreamWriter class? The reader of course allows for very efficient, async reading from the stream and can return ReadOnlySequence<byte> slices with entire msgpack structures in it. The writer could implement IBufferWriter<byte> so that it can be used to serialize with MessagePackSerializer directly. It might use Sequence<byte> internally for buffering but would allow us to be writing to the stream in the background even while serialization occurs.
    Andrew Arnott
    @AArnott
    I think if we remove the Stream-related APIs from MessagePackSerializer then folks would look at it just as a serializer and not as a streaming control system as well, and the two class names I propose would be pretty discoverable, IMO. As people type MessagePackS they would see the Serializer as well as the StreamReader/Writer classes in the completion list. But if we wanted it even more discoverable, we could add static CreateStreamReader and CreateStreamWriter methods to the MessagePackSerializer class.
    Or here's another option that you might like: just add a ref StreamReaderState parameter to the Deserialize(Stream method that folks would repeatedly pass in each time they read from the same stream, and that would allow us to incrementally read from the stream. This struct would have a bunch of internal fields that would retain the state we need (e.g. the buffers we read too far on). The only question would be what we do with DeserializeAsync since async methods can have ref parameters. We could use a class for StreamReaderState instead of a struct to allow the async method to work.
    Andrew Arnott
    @AArnott
    Here's a draft of the alternative solution I was describing.
    Yoshifumi Kawai
    @neuecc

    I commented on PR, but I think State is a good idea.
    It's a somewhat uncommon API, but it should be fine.

    Even if there is a State, I think that it is not bad to default to reading until the end if it is not specified.
    Here is my image of usage:
    90%-Ok to read end.
    9%-simply contiguous block.
    1%-concatenated other things.

    90% situation, read to end is fastest.
    9% situation, use state is good to use.
    1%, we don't care.

    Andrew Arnott
    @AArnott
    Sounds good, @neuecc. I'll proceed with the state API design. And we can keep overloads that omit the state parameter to keep it simply and familiar for folks in your 90% case.
    For the async method where we can't use ref parameters, I can use a StrongBox<state> parameter to achieve the same result.
    Aleksander Heintz
    @Alxandr
    @AArnott why *Strong*Box?
    And not just Box
    Or, AsyncRef<T>
    Andrew Arnott
    @AArnott
    @Alxandr I've never heard of Box or AsyncRef<T>. Are those existing types? StrongBox<T> is an existing type and why I picked it. But anyway it's a moot point because #699 is now complete. We don't need any of these solutions.
    FWIW though, I think the .NET team called it StrongBox<T> because boxing is something that happens automatically all the time, but this "strong box" forces a box around a value type that won't come and go automatically. It's all explicit. So I guess that's why they called it "strong".
    Andrew Arnott
    @AArnott
    @neuecc I am going to try to find some time to sift through the nearly 200 open issues on MessagePack and close the old ones or the ones I think are fixed, or that very few people care about. I think 200 is just too many to really manage, and if we can get it down to less than 40, we can do a better job prioritizing work. If I close too many, the folks who care will make a case to reactivate them.
    Andrew Arnott
    @AArnott
    @neuecc Can you please review #703 and let me know whether you think 2.0 or 2.1 is more appropriate?
    Yoshifumi Kawai
    @neuecc
    @AArnott commented. I think, it will go 2.1. 2.0 has been confirmed that the core has been stable for a week. I think we can ship it.
    Aleksander Heintz
    @Alxandr
    @AArnott Ah. I thought we were making simple wrappers. I have AsyncRef and AsyncOut types in some projects which are just structs that you can write (and in the case of ref) read from.
    Or rather, classes
    I didn't know StrongBox was a existing type
    Andrew Arnott
    @AArnott
    The 2.0 stable package is building, and will push to nuget.org in moments... 🎉
    Yoshifumi Kawai
    @neuecc
    🎉🎉🎉
    Andrew Arnott
    @AArnott
    @neuecc Why is it important for us to build messagepack.dll for Unity within Unity (thus limiting us to C# 7.3)? If Unity can load other netstandard2.0 assemblies that were compiled outside Unity, couldn't we leverage this for messagepack.dll as well?
    Yoshifumi Kawai
    @neuecc

    @AArnott
    1.
    Standard way of publish unity assets is source code level(many unity asset stores libraries, unity package manager).
    It would be the reason why Unity chose UPM (Fork of NPM) instead of NuGet.

    2.
    for net46 and netstandard20 in single unitypackage.
    I want to support use dynamic assembly if can(in UnityEditor, PC, etc.).
    net46 has ref-emit but netstandard20 requires external library.
    In includes external library(System.Reflection.Emit), it occurs error on net46.
    This can be solved by separating unitypackage, but one unitypackage is preferable.

    3.
    To resolve conflict of assembly dependency.
    For example, Unsafe.dll was conflicted.
    In the case of publishing source code,
    it was possible to delete the included dll,
    but in the case of messagepack.dll,
    warning is unavoidable (like the warning in Nerdabnk.Streams before)
    Andrew Arnott
    @AArnott
    Thanks for the explanation. Sharing unity assets that include non-DLL content makes sense to share as content. I hope that at some point the Unity toolset will mature to the point where messagepack.dll can be shared instead. Unsafe hopefully is a point in time thing. Using runtime checks to avoid dynamic ref emit wherever it's not allowed could also help us not have to use custom compilations.
    Andrew Arnott
    @AArnott
    It looks like our Unity build agent has been down for 12 days. In the meantime I suppose all PRs had failed or stalled builds, and at least one was completed that broke the build (#739). I'll work on a fix and probably direct push.
    Yoshifumi Kawai
    @neuecc
    Andrew Arnott
    @AArnott
    Nice. One typo you may want to fix @neuecc : Pipleines -> Pipelines
    Yoshifumi Kawai
    @neuecc
    thanks!
    Yoshifumi Kawai
    @neuecc
    @AArnott Do you have any plan to next patch release?
    I want to ship some fixes about mpc that already merged to master.
    Andrew Arnott
    @AArnott
    patch release, or minor release? master would be the next minor release (2.1). We also have some fixes in the v2.0 branch that we could release as a patch to 2.0. Which are you eager to release?
    Yoshifumi Kawai
    @neuecc

    @AArnott
    I only care about mpc, so I think patch is appropriate (major, minor will need to be more considered).
    It was a failure for me(and contributor) to choose 2.0 for some PR.
    However, it may be a difficult rule for contributors to choose 2.0 as the base of the patch for Bug Fix.

    Abou mpc:
    neuecc/MessagePack-CSharp#739
    neuecc/MessagePack-CSharp#741
    neuecc/MessagePack-CSharp#742

    Not about mpc, but can become patch(?)
    (I received some users uses CompositeResover.Create and does not work on IL2CPP(warning is not noticed!)
    neuecc/MessagePack-CSharp#754

    Andrew Arnott
    @AArnott
    So are you suggesting that these 4 PRs which merged into master should also be merged into v2.0 in order to release as a 2.0 patch? I tend to like to get ahead of these things during the PR by encouraging folks to rebase on v2.0, even if targeting master so that if they change their minds and want to merge into v2.0 we are free to do so without having to 'cherry-pick' commits which makes tracking fixes harder. But I'm sure we can special case these and copy the changes to v2.0 if they're stable changes that fix things without adding API.

    it may be a difficult rule for contributors to choose 2.0 as the base of the patch for Bug Fix

    We could fix this by making v2.0 the 'default' branch in the repo.

    Yoshifumi Kawai
    @neuecc

    So are you suggesting that these 4 PRs which merged into master should also be merged into v2.0 in order to release as a 2.0 patch?

    if next minor release is not soon, yes.

    Andrew Arnott
    @AArnott
    I don't mind another minor release being soon. Let's sift through these 2.1 milestone issues and take some out and put some in as necessary to make sure they're the right set for whatever we can do "soon" and get 2.1 released. :)
    Yoshifumi Kawai
    @neuecc
    At least Deferred support will be no implementation:)
    Andrew Arnott
    @AArnott
    If you mean that's a candidate for deferral to another milestone, I agree. It will be non-trivial.