plone / guillotina

Python AsyncIO data API to manage billions of resources
https://guillotina.readthedocs.io/en/latest/
Other
187 stars 50 forks source link

Deserialize pickles in a asyncio executor + converted db functions reader() and IWriter.serialize() to async #1171

Open masipcat opened 2 years ago

masipcat commented 2 years ago

I measured that big objects >50kB take a lot of time to deserialize, about 50, 100, 200ms depending on the size.. This change avoids blocking all requests in the process during deserialization.

I'm not sure if we should consider this a breaking change or if it's ok as a patch version

codecov-commenter commented 2 years ago

Codecov Report

Merging #1171 (7c1b5d2) into master (ee38eea) will increase coverage by 0.1%. The diff coverage is 96.6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1171     +/-   ##
========================================
+ Coverage    94.6%   94.6%   +0.1%     
========================================
  Files         377     377             
  Lines       32650   32658      +8     
========================================
+ Hits        30873   30881      +8     
  Misses       1777    1777             
Impacted Files Coverage Δ
guillotina/_settings.py 100.0% <ø> (ø)
guillotina/db/transaction.py 89.8% <85.8%> (ø)
guillotina/db/reader.py 100.0% <100.0%> (ø)
guillotina/db/storages/cockroach.py 80.0% <100.0%> (ø)
guillotina/db/storages/dummy.py 94.0% <100.0%> (ø)
guillotina/db/storages/pg.py 85.9% <100.0%> (ø)
guillotina/db/writer.py 95.4% <100.0%> (ø)
guillotina/factory/content.py 83.3% <100.0%> (ø)
guillotina/tests/cache/test_cache_memory.py 100.0% <100.0%> (ø)
guillotina/tests/mocks.py 85.4% <100.0%> (ø)
... and 2 more
masipcat commented 2 years ago

IMO, this is a breaking change. Lots of interfaces that are potentially used.

OK, agree

We could make the interface optionally async. Use https://github.com/plone/guillotina/blob/master/guillotina/utils/misc.py#L126 to not cause breaking change

I'm ok with this change but third-party applications using the guillotina IWriter will still break so maybe we should update the minor version?

vangheem commented 2 years ago

maybe we should update the minor version

Yes, I agree.