aiopg icon indicating copy to clipboard operation
aiopg copied to clipboard

Rework aiopg.sa.result module with Cython.

Open AndreiPashkin opened this issue 4 years ago • 8 comments

I've noticed that classes ResultProxy and RowProxy are bottlenecks in one of my projects and that other libraries like SQLAlchemy or Asyncpg implement similar classes in C for speed [1], [2].

I decided to start experimenting with doing the same thing in Aiopg - I've done some simple optimizations using Cython and already got some positive results using this benchmark setup: https://gist.github.com/AndreiPashkin/dd6aa232f91deaa987b5ee0285ad9b7d

Results before:

simple [1.3329865639971104, 1.3546994640055345, 1.349644674002775]
json [1.6616086649883073, 2.042356903999462, 2.0650724380102474]

Results after:

simple [0.5680085269996198, 0.5687800829909975, 0.5716462380078156]
json [0.8984643409930868, 0.8978340939938789, 0.8937820449937135]

What do maintainers think about this? I it worth to continue? Maybe it's even worth to simply re-use fast ResultProxy implementation from SQLAlchemy?

Are there changes in behavior for the user?

Theoretically everything should be backwards-compatible

Related issue number

N/A

Checklist

  • [x] I think the code is well written
  • [x] Unit tests for the changes exist
  • [ ] Documentation reflects the changes
  • [ ] Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

AndreiPashkin avatar Mar 18 '20 10:03 AndreiPashkin

This pull request fixes 1 alert when merging f8ab48abffc228c3b69979ff00064245f7504ae8 into c83e6a5a93ceeb71fc08aff459e7d67aaf1fefa3 - view on LGTM.com

fixed alerts:

  • 1 for Comparison using is when operands support __eq__

lgtm-com[bot] avatar Mar 18 '20 10:03 lgtm-com[bot]

Codecov Report

Merging #664 into master will increase coverage by 0.16%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #664      +/-   ##
==========================================
+ Coverage   92.18%   92.35%   +0.16%     
==========================================
  Files          10        9       -1     
  Lines        1037      837     -200     
  Branches      121       88      -33     
==========================================
- Hits          956      773     -183     
+ Misses         65       56       -9     
+ Partials       16        8       -8     

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c83e6a5...509c53c. Read the comment docs.

codecov[bot] avatar Mar 18 '20 10:03 codecov[bot]

@asvetlov, couple of concrete questions:

  1. Is ability to index by SQLAlchemy columns really needed? Can it be dropped?
  2. Can lazy processing of row values with processors be replaced with eager processing?

AndreiPashkin avatar Mar 18 '20 15:03 AndreiPashkin

This pull request fixes 1 alert when merging b6ac0257523ea6d9add136ba9c2a75ac736b8fa7 into c83e6a5a93ceeb71fc08aff459e7d67aaf1fefa3 - view on LGTM.com

fixed alerts:

  • 1 for Comparison using is when operands support __eq__

lgtm-com[bot] avatar Mar 18 '20 15:03 lgtm-com[bot]

This pull request fixes 1 alert when merging 420ee855134e3bf336940807237d841aa64356cb into c83e6a5a93ceeb71fc08aff459e7d67aaf1fefa3 - view on LGTM.com

fixed alerts:

  • 1 for Comparison using is when operands support __eq__

lgtm-com[bot] avatar Mar 20 '20 15:03 lgtm-com[bot]

This pull request fixes 1 alert when merging db954215a8b4e0de12785c198dc5aef41d707106 into c83e6a5a93ceeb71fc08aff459e7d67aaf1fefa3 - view on LGTM.com

fixed alerts:

  • 1 for Comparison using is when operands support __eq__

lgtm-com[bot] avatar Mar 23 '20 14:03 lgtm-com[bot]

This pull request fixes 1 alert when merging 509c53cbea8aeb3bc85bb4f0efca8570561c5119 into 3fb3256319de766384b4867c7cc6710397bd1a8c - view on LGTM.com

fixed alerts:

  • 1 for Comparison using is when operands support __eq__

lgtm-com[bot] avatar Mar 31 '20 15:03 lgtm-com[bot]

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Nov 16 '20 10:11 CLAassistant