Yelp / bravado-core

Other
109 stars 98 forks source link

bravado_core should use the msgpack module instead of umsgpack #227

Closed AlstonLin closed 6 years ago

AlstonLin commented 6 years ago

It seems that the umsgpack module, implemented in pure python, is an order of magnitude slower than the msgpack module, written with CPython bindings

gstarnberger commented 6 years ago

IIRC the original msgpack module is not well supported under PyPy. Do you think it would make sense to add a fallback to umsgpack? Something like the following:

try:
    import msgpack
except ImportError:
    import umsgpack as msgpack

In that case we could specify umsgpack as required dependency (as it should be available for all Python versions) and msgpack as optional dependency.

macisamuele commented 6 years ago

@gstarnberger thanks for reporting it .... I was almost sure that I've read somewhere that it was preferable to use umsgpack over msgpack 😄

@AlstonLin I would keep into consideration @gstarnberger proposal, bravado has already something similar for json, maybe we can "replicate" that approach.

AlstonLin commented 6 years ago

@gstarnberger According to https://github.com/msgpack/msgpack-python, msgpack-python is supported with Pypy (there's even a section for it explicitly stating that in the README). I thought I read somewhere as well that umsgpack was more preferable but I guess that was incorrect. Since there's msgpack-python is fully supported on Pypy using your approach would not be necessary.

AlstonLin commented 6 years ago

@macisamuele @sjaensch when would it be possible to get a new release for this to go out so we can start trying these out in our clientlibs?