Yelp / bravado-core

Other
109 stars 98 forks source link

Enhance unmarshal performances #331

Closed macisamuele closed 5 years ago

macisamuele commented 5 years ago

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:

  1. extract the type from the schema
  2. 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
  3. 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 in the next major release. Doing so will allows us to modify more, if needed, the internals of unmarshaling without worrying about breaking backward compatibility.

Be aware that:

This PR provides a massive performance improvement:

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-0.2%) to 98.065% when pulling c98a9c03ce95ee118334949901969d1a66cfef1c on macisamuele:maci-unmarshal-performances into c48be8acf013628c6eb8f434f6e24390c1ad4143 on Yelp:master.

macisamuele commented 5 years ago

I'm closing this PR as the history is a bit confusing. A new PR, with the same changes, will be created and linked to this