MessagePack.swift icon indicating copy to clipboard operation
MessagePack.swift copied to clipboard

Swift4 Encoder/Decoder

Open Bersaelor opened this issue 6 years ago • 16 comments

Hey Alexsander,

so I recently started adding the Codable protocol conformance to my KDTree framework.

After I finished the conformance to Encodable/Decodable and tested it with Apples JSONEncoder and PropertyListEncoder I noticed that decoding a tree from file took about 3 times as long as just loading my plain data from a csv AND building the tree again. I know it's because the Apple Encoders use NSNumber and NSString internally which makes them slow. But I'm having a hard time accepting that loading the tree structure from a file is slower then running through all the sorting required to create a new tree.

So, my question to you, are you planning to update MessagePack.swift so that it will Encode/Decode Codable swift files? I feel with Swift4 there will be a big need for faster alternatives to the JSONEncoder and PropertyListEncoder Apple provides :)

Thank you!

Bersaelor avatar Aug 29 '17 09:08 Bersaelor

Hey @Bersaelor, this wasn't on my roadmap but I think this is a great idea! Anyone should feel free to start a pull request if I don't have time right away.

a2 avatar Aug 29 '17 13:08 a2

@a2 Do you have a few tips on performance?

I was doing some preliminary testing with the following code on a large test array:

writing:

let packedStars = starArray.map { $0.messagePackValue() }
let data = pack(.array(packedStars))
try data.write(to: filePath, options: Data.WritingOptions.atomic)

and reading:

let unpacked = try unpack(data).value
let stars = unpacked.arrayValue!.map { Star(value: $0) }

It only takes 40% as much as reading this array from a json, but it's still disappointingly slow. How can I write this in a more streamed way, so that I can read each element in the array and transform it individually? Just the try unpack(data).value step takes ~3s for a 12.5MB file. I have the same data available as a 33MB csv-file, which only takes 1.5s to read and map into the resulting Star-valued array. The Messagepack must be faster then the file that is stored as plain text, so I must be doing something wrong.

Bersaelor avatar Sep 05 '17 20:09 Bersaelor

@Bersaelor I'm not sure if you're doing anything wrong but I'm also sure there are places ripe for optimization. Maybe it's worth running the code under Instruments.app to see where the slow parts are? I'm not sure off the top of my head what would be extra slow about it.

a2 avatar Sep 06 '17 08:09 a2

@a2 I just stumbled across this ticket. I've actually been working on an implementation of a swift Encoder/Decoder for MessagePack over the weekend. It's not quite ready to be published to github, but I should be done in the next week or so.

mgadda avatar Sep 24 '17 23:09 mgadda

@mgadda That sounds awesome! Is the Codable support based on MessagePack.swift or is it another library, if I may ask?

a2 avatar Sep 25 '17 09:09 a2

Sorry, I didn't mean to be unclear. Yes, it's based on (i.e. dependent upon) your MessagePack.swift. And in terms of structure it closely follows the patterns established in JSONEncoder/Decoder.swift and PropertyListEncoder/Decoder.swift. Where those encoders would yield NSObjects, MessagePackEncoder (privately) yields a MessagePackValue. The public API of course returns a Data instance.

mgadda avatar Sep 25 '17 17:09 mgadda

@mgadda Awesome. Feel free to contribute it to this repo as a PR or maybe it should be a separate repo. Not sure what the pros/cons of that would be. Thoughts?

a2 avatar Sep 25 '17 18:09 a2

I could go either way. Let me just finish up the implementation and then we can figure out where it should live.

mgadda avatar Sep 27 '17 14:09 mgadda

hi, i have also implemented a MesagePackEncoder/Decoder as I will be using MessagePack in my new project and converting my models manually is a pain. My implementation heavily references JSONEncoder.swift and it turns out that the implementation is rather involved and non-trivial.

I have submitted a PR #61 but not sure if this is the best way forward as this project is very lightweight.

andrew-eng avatar Oct 03 '17 12:10 andrew-eng

Wow! @andrew-eng, this looks incredible. Thanks for your hard work to port this from JSON to MessagePack. I know @mgadda was working on another implementation of this same thing. Matt, what are your thoughts on this implementation? I think you have more context than I do about this kind of thing since you were working on it too. I'd appreciate the help, not to mention the support of the community, in this matter. 😄

a2 avatar Oct 03 '17 13:10 a2

Oh, that’s too bad. Seems like i was too slow! The implementation is nearly complete now. It too follows the same patterns shown in JSONEncoder.swift and is of course quite involved. Should I abandon course? On Tue, Oct 3, 2017 at 06:06 Alexsander Akers [email protected] wrote:

Wow! @andrew-eng https://github.com/andrew-eng, this looks incredible. Thanks for your hard work to port this from JSON to MessagePack. I know @mgadda https://github.com/mgadda was working on another implementation of this same thing. Matt, what are your thoughts on this implementation? I think you have more context than I do about this kind of thing since you were working on it too. I'd appreciate the help, not to mention the support of the community, in this matter. 😄

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/a2/MessagePack.swift/issues/54#issuecomment-333835508, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUSe8RioUS9xz_xKR-LBlRTV7-6E6Hks5sojFZgaJpZM4PFmN- .

mgadda avatar Oct 07 '17 19:10 mgadda

@mgadda Hmmm, it might make sense to abandon course. I'd like to merge @andrew-eng's PR but I'm not 100% sure how everything works nor if it's completely done. 😬 Perhaps you could take a look at it and let me know if you think it's ready to go? Then we can merge it and get it ready for a new release version.

a2 avatar Oct 09 '17 09:10 a2

@a2 I'll do a robust check on the PR before we merge it in. @mgadda has highlighted a few issues with the current PR and I'll work with him to resolve them.

Btw @mgadda, possible to share your implementation with me? 😁 Quite curious to know how you do it. Initially i tried writing form scratch but soon realized that i end up with the same structure as JSONEncoder.swift after accounting for all the complications. Then I realized that they pretty much copy-pasted the entire code into PlistEncoder.swift, and so I did the same too in this PR.

andrew-eng avatar Oct 10 '17 08:10 andrew-eng

I'm new at programming, so sorry if the question is stupid. Is there a way to pack a whole class, and not do every single property separately? For example: class Person{

var name: String
var lastName: String
var age: Int
}

let me = Person(name:"First", lastName: "Last", age: 2332)

let packed = me.pack ... var unpacked = receivedData.unpack

or something like this...

B00jan avatar Mar 27 '18 07:03 B00jan

@B00jan Unfortunately, this is not currently possible.

a2 avatar Mar 27 '18 08:03 a2

Ok, thank you for your fast response!

B00jan avatar Mar 27 '18 08:03 B00jan