Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

faster creation of PyList #74

Closed
wants to merge 1 commit into from
Closed

faster creation of PyList #74

wants to merge 1 commit into from

Conversation

samuelcolvin
Copy link
Member

No description provided.

Copy link

codspeed-hq bot commented Mar 28, 2024

CodSpeed Performance Report

Merging #74 will not alter performance

Comparing fast-pylist (6e80788) with main (f6b698e)

Summary

✅ 59 untouched benchmarks

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.11%. Comparing base (37f9138) to head (6e80788).
Report is 60 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
- Coverage   96.59%   95.11%   -1.49%     
==========================================
  Files           9       10       +1     
  Lines        1294     1494     +200     
==========================================
+ Hits         1250     1421     +171     
- Misses         44       73      +29     
Files with missing lines Coverage Δ
src/python.rs 97.18% <100.00%> (+0.26%) ⬆️

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@samuelcolvin
Copy link
Member Author

samuelcolvin commented Mar 28, 2024

This makes a substantial difference locally, but codspeed seems to disagree

image

???

@davidhewitt
Copy link
Collaborator

Is "locally" arm or x86_64? Might differ greatly between the two platforms.

TBH I'm ok with this but I also just think that really PyO3 should be reworking its list constructor to allow this.

@samuelcolvin
Copy link
Member Author

Locally was macos.

@samuelcolvin
Copy link
Member Author

Not clear there's an improvement on x86, might even be worse (although I have no idea how it could be):

image

So I'll leave this, we can either reconsider or close.

@samuelcolvin samuelcolvin marked this pull request as draft April 2, 2024 11:50
@davidhewitt
Copy link
Collaborator

I think better is that I aim to fix PyO3 to avoid needless reference count operations. Would be a modest speedup for quite a few operations / smaller binary size :)

@davidhewitt
Copy link
Collaborator

PyO3 0.23 (#137) has a new IntoPyObject trait which optimises transfer of reference counts, will close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants