NOTE: This CR replaces #331 . This is done due to the fact that history on the other PR is quite confusing.
The goal of this PR is, as mentioned in the title, unmarshaling performance enhancement.
NOTE: A similar PR will be published for applying the same approach described here for the marshaling process. I would like to have this verified first in order to avoid having two massive branches open at the same time
In order to achieve unmarshaling performance boost I've rewritten the whole bravado_core.unmarshal module.
The current, before this PR, logic of unmarshaling is:
extract the type from the schema
if the schema if of type x then call the appropriate unmarshaling function
if the schema is of type object: call the top unmarshaling function for the property (point 1)
if the schema is of type object: call the top unmarshaling function for each item of the list (point 1)
if the schema is of primitive type: identify, each time, the right formatting function to use
once the unmarshaling function is identified then perform the real unmarshaling
This PR aims to cache results of point 1 and 2 for later "cost-free" usage.
While making this changes I've noticed that there might be no real good reason to have all the unmarshaling functions public, users of the library commonly use bravado_core.unmarshal.unmarshal_schema_object.
In order to simplify future maintenance work I've marked all the other functions as deprecated and will be removed in the next major release. Doing so will allows us, if needed, to modify more the internals of unmarshaling without worrying about breaking backward compatibility.
This PR provides a massive performance improvement:
the worst performance improvement is ~46% (if in the branch takes 1 then on master takes 1.87)
the best performance improvement is ~70% (if in the branch takes 1 then on master takes 3.33)
the average performance improvement is ~60% (if the branch takes 1 then on master takes 2.57)
Be aware that:
bravado_core.unmarshal.get_unmarshaling_method is wrapped by _decorators.wrap_recursive_call_exception and memoize_by_id decorators.
_decorators.wrap_recursive_call_exception decorator is needed as models could be recursive and so invoking get_unmarshaling_method might end up on an infinite recursion.
In order to proactively deal with it I've modified @memoize_by_id decorator to raise a controlled exception if an unbounded recursion is identified (ℹ️ the interpreter would raise anyway with a RuntimeError: maximum recursion depth exceeded)
memoize_by_id decorator is needed to speed up evaluation of the marshaling method for the same spec and schema. This effectively is one of the major performance boost (basically the expensive point 1 and 2 of the original flow become, more or less, a dictionary lookup)
All the test changes have been made before the refactoring (except tests/unmarshal/unmarshal_object_test.py) to guarantee that functionally nothing changed ;)
Coverage decreased (-0.5%) to 97.771% when pulling c804bc10b880dd758dd5c1617b6b46f89b1aa75c on macisamuele:maci-enhance-unmarshaling-performances into c48be8acf013628c6eb8f434f6e24390c1ad4143 on Yelp:master.
NOTE: This CR replaces #331 . This is done due to the fact that history on the other PR is quite confusing.
The goal of this PR is, as mentioned in the title, unmarshaling performance enhancement. NOTE: A similar PR will be published for applying the same approach described here for the marshaling process. I would like to have this verified first in order to avoid having two massive branches open at the same time
In order to achieve unmarshaling performance boost I've rewritten the whole
bravado_core.unmarshal
module.The current, before this PR, logic of unmarshaling is:
This PR aims to cache results of point 1 and 2 for later "cost-free" usage.
While making this changes I've noticed that there might be no real good reason to have all the unmarshaling functions public, users of the library commonly use
bravado_core.unmarshal.unmarshal_schema_object
. In order to simplify future maintenance work I've marked all the other functions as deprecated and will be removed in the next major release. Doing so will allows us, if needed, to modify more the internals of unmarshaling without worrying about breaking backward compatibility.Let's get to the juicy part 😄 Raw data on: https://gist.github.com/macisamuele/9ff23d3e5e81c40cd884425a7eae5751
This PR provides a massive performance improvement:
Be aware that:
bravado_core.unmarshal.get_unmarshaling_method
is wrapped by_decorators.wrap_recursive_call_exception
andmemoize_by_id
decorators._decorators.wrap_recursive_call_exception
decorator is needed as models could be recursive and so invokingget_unmarshaling_method
might end up on an infinite recursion. In order to proactively deal with it I've modified@memoize_by_id
decorator to raise a controlled exception if an unbounded recursion is identified (ℹ️ the interpreter would raise anyway with aRuntimeError: maximum recursion depth exceeded
)memoize_by_id
decorator is needed to speed up evaluation of the marshaling method for the same spec and schema. This effectively is one of the major performance boost (basically the expensive point 1 and 2 of the original flow become, more or less, a dictionary lookup)tests/unmarshal/unmarshal_object_test.py
) to guarantee that functionally nothing changed ;)