bloom-lang / bud

Prototype Bud runtime (Bloom Under Development)
http://bloom-lang.net
Other
856 stars 59 forks source link

Automatically fall back to Marshal when marshaling tuples #295

Closed JoshRosen closed 11 years ago

JoshRosen commented 11 years ago

This commit changes the field marshaler to automatically fall back to Marshal if MessagePack cannot serialize a field. The old workaround failed when marshaling Ruby objects that contained nested lattices (e.g. a lattice in an array); this commit fixes that problem.

This is still a bit of a hack, but it maybe it's preferable to completely replacing MessagePack with Marshal.

neilconway commented 11 years ago

Seems reasonable, although I wonder if we just shouldn't switch to Marshal unconditionally. Using Marshal also fixes stuff like Symbols not roundtripping through MsgPack correctly:

1.9.3p374 :003 > MessagePack.unpack(:foo.to_msgpack)
 => "foo" 

Downside is a slight performance hit, but maybe that's not worth the trouble/complexity of trying to use both MsgPack and Marshal...

jhellerstein commented 11 years ago

We have not worried about raw performance since the initial push-based runtime thrust.

It's worth a little microbenchmark to see if MsgPack vs. Marshal is noticeable -- and if not, where the bottlenecks are these days.

J

On Feb 18, 2013, at 8:31 PM, Neil Conway notifications@github.com wrote:

Seems reasonable, although I wonder if we just shouldn't switch to Marshal unconditionally. Using Marshal also fixes stuff like Symbols not roundtripping through MsgPack correctly:

1.9.3p374 :003 > MessagePack.unpack(:foo.to_msgpack) => "foo" Downside is a slight performance hit, but maybe that's not worth the trouble/complexity of trying to use both MsgPack and Marshal...

— Reply to this email directly or view it on GitHub.

neilconway commented 11 years ago

I did a microbenchmark a few months back: https://groups.google.com/d/msg/bloom-lang/i6vMaUOC99A/1QLSrNANGHAJ ; the performance penalty was about 5%.

neilconway commented 11 years ago

For now it seems best to just merge this -- if we want to switch over to pure Marshal down the road we can always do that, but for now this is a clear improvement on what we had before.

Thanks for the patch, Josh!