mbraceproject / FsPickler

A fast multi-format message serializer for .NET
http://mbraceproject.github.io/FsPickler/
MIT License
326 stars 52 forks source link

Lists are not serilized as Json array #19

Closed thinkbeforecoding closed 10 years ago

thinkbeforecoding commented 10 years ago

for instance

type Rec = { value: int list }
pick.PickleToString { value = [1;2;3] }

outputs

{"value":{"length":3,"list":[1,2,3]}}

It would be better to have something like:

{"value": [1,2,3] }
thinkbeforecoding commented 10 years ago

tell me if it's what's expected, I can see if I can solve it on my side and send a PR...

eiriktsarpalis commented 10 years ago

This is by design. Prefixing the length of lists/arrays usually results in more efficient deserialization. I must admit that this is mostly an artifact when the library was restricted to binary serialization, so it is definitely worth checking if this could be improved in the case of json. That said, you should keep in mind that picklers are format-agnostic, so any changes should not break e.g. the xml format.

thinkbeforecoding commented 10 years ago

Yes, this is what I noticed by reading the code. Would it be possible to change to use another pickler generator when using json, maybe ? The pickler would not know about the serializer, but the pickler autogenerator would be different for json... makes sense ?

eiriktsarpalis commented 10 years ago

Sounds a bit too drastic. The pickler cache is heavyweight since it involves dynamic method compilations. Now, I do have a couple of other ideas in mind but I will need to check the performance impact first.

thinkbeforecoding commented 10 years ago

The thing is that tags are written by BeginWriteObject which write both the tag and start an object.. There is no separation between the field and the fact that what's after can have a different shape..

eiriktsarpalis commented 10 years ago

I'm not entirely sure how what you're saying is related to the array issue.

eiriktsarpalis commented 10 years ago

Ok, I now see what you're saying.

The decision to wrap every non-primitive in a Begin/EndWriteObject is deliberate. This happens since occasionally FsPickler includes metadata on the object: if it is null, cached, an instance of an inheriting class that requires a different pickler or even part of a cyclic dependency. For example,

type Rec = { Rec : Rec }
let rec f = { Rec = f }
let p = Json.pickle Pickler.auto f // {"Rec":{"pickle flags":"cyclic","id":1}}

Silly as it may look, such patterns are prevalent in the .NET world, even in types that are supposedly serializable (Look no further than System.Globalization types). I hope that this proves to you that metadata is essential if .NET serialization compatibility is to be maintained.

Back to array serialization, I remembered another reason why prefixing lengths is essential: cyclic arrays. Consider the example:

let x = [|obj() ; obj()|]
x.[0] <- box x

The only way to properly deserialize this is to know the length of the array a priori.

thinkbeforecoding commented 10 years ago

I managed to tweak the JsonPickleWriter to serialized lists as simple Json array..

But it won't work with the deserializer, since the pickled explicitly wait for the length before reading any value.. Arrays and lists have to be written as unbounded sequences.. but it require more changes...

thinkbeforecoding commented 10 years ago

I see.. It's true that FsPickler can serialize things that other serializers cannot ! Still it could be interesting for Json to serialized as simple array when possible to be more idiomatic.. especially when List, arrays cannot be cyclic...

thinkbeforecoding commented 10 years ago

I was thinking to create my own specific picklers using Pickler.FromPrimitives, but it's quite useless because Formatter is internal... So reader and writers have access to almost nothing on ReadState/WriteState....

eiriktsarpalis commented 10 years ago

Ok, so after much thought, I came up with this solution:

https://github.com/nessos/FsPickler/tree/format-experiments

This should serialize lists and some arrays as expected in the Json format. It required quite a bit of refactoring, so I still consider this to be an experimental change. Give it a try and let me know what you think!

eiriktsarpalis commented 10 years ago

Just pushed to the master, so I'm just going to close this