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

Start adding a performance section to the guide. #3304

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Conversation

adamreichold
Copy link
Member

We discussed this quite often and I though that even if it is only a single thing, just making a start would improve our chances of getting it finished eventually.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for kicking this off!

One day it would be nice to remove this performance pitfall. Something to investigate after removing the pool 😄

newsfragments/3304.added.md Show resolved Hide resolved
guide/src/performance.md Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member Author

@davidhewitt I added another subsection on implicit access to GIL token. Please have another look before this is merged.

Also do you remember any other topics when wanted to discuss here? I do think I remember that there about four different things we wanted to have here, but I am unable find a list anywhere in the older issues, pull requests or discussions.

@adamreichold
Copy link
Member Author

Also do you remember any other topics when wanted to discuss here?

Is dictionary dispatch, i.e. look-up tables based on type object identity, something we want to discuss here or is this too specialized?

@davidhewitt
Copy link
Member

Personally I think that's quite application-specific, but I'm not against having that section if you want to write it.

Some other topics I recalled over the day:

  • Overhead of conversions (i.e. PyList vs Vec etc.)
  • Threading / allow_threads
  • String intern!
  • Vec<u8> becoming list[int] (and thus being very slow)
  • #[pyo3(get)] deep-cloning non-Py data

We can potentially move these into an issue rather than write them all in this PR?

@adamreichold
Copy link
Member Author

Threading / allow_threads

Shouldn't this be covered by the "parallelism" section?

We can potentially move these into an issue rather than write them all in this PR?

Yes, I'd like to limit this PR to the two small sections already written.

Personally I think that's quite application-specific, but I'm not against having that section if you want to write it.

Ok, let's include a note in the issue. Certainly not in this PR.

@adamreichold adamreichold added this pull request to the merge queue Jul 11, 2023
Merged via the queue into main with commit 89b9bc3 Jul 11, 2023
@adamreichold adamreichold deleted the guide-perf-section branch July 11, 2023 20:22
@davidhewitt
Copy link
Member

Sorry for the slow reply - I got a bit sidetracked by the various PRs I have pushed in the last 24h...

Shouldn't this be covered by the "parallelism" section?

Yes, though I think it can't hurt to call it out i.e. "how do I multithread? See parallelism section". More visibility :)

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