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

Speed up os.path #117349

Closed
Tracked by #117361
nineteendo opened this issue Mar 28, 2024 · 4 comments
Closed
Tracked by #117361

Speed up os.path #117349

nineteendo opened this issue Mar 28, 2024 · 4 comments
Labels
type-feature A feature request or enhancement

Comments

@nineteendo
Copy link
Contributor

nineteendo commented Mar 28, 2024

Feature or enhancement

Proposal:

There are some minor optimisations possible in os.path, including some fast returns.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Links to previous discussion of this feature:

Linked PRs

@nineteendo nineteendo added the type-feature A feature request or enhancement label Mar 28, 2024
@nineteendo nineteendo mentioned this issue Mar 29, 2024
16 tasks
AlexWaygood added a commit that referenced this issue Apr 2, 2024
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Barney Gale <barney.gale@gmail.com>
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
@nineteendo
Copy link
Contributor Author

@barneygale, can you reopen this?

@AlexWaygood
Copy link
Member

Hey @nineteendo. In my opinion, optimisations for os.path should be welcome where they don't lead to significant deteriorations in style -- these are low-level functions that can often be used many times across a codebase, so improving performance of these functions can be pretty significant for end users of Python. Quoting my comment from #117350 (comment), however:

My advice for the future, however, would definitely be to work on small, focused PRs that have isolated, easily measurable changes. PRs like that are much easier for us to review, and you should find the contributing experience less frustrating as a result.

If you have a specific optimisation in mind (that improves performance by 5% or more, perhaps) for a specific function, and you're confident that the full test suite passes with the optimisation applied, then I would advise opening an issue regarding just that one optimisation. The issue should describe the proposed optimisation properly ("fast returns" isn't really a sufficient description here :-). Any PRs linked to that issue should tackle the specific optimisation proposed in the issue, and nothing else. Generic issues that propose "optimising os.path", and PRs that make changes to multiple functions at once, aren't particularly helpful ways to engage in CPython development.

@nineteendo
Copy link
Contributor Author

If you have a specific optimisation in mind (that improves performance by 5% or more, perhaps) for a specific function, and you're confident that the full test suite passes with the optimisation applied, then I would advise opening an issue regarding just that one optimisation.

If the same optimisation can be applied to several functions, can I make a single issue?

Any PRs linked to that issue should tackle the specific optimisation proposed in the issue, and nothing else.

Does that include further optimisations to the modified functions, besides the proposed one? Or do I need to make a separate issue?

@AlexWaygood
Copy link
Member

If the same optimisation can be applied to several functions, can I make a single issue?

I think that would be fine. But I'd make sure to limit any PR so that it only changes functions where that optimisation actually makes a difference :-)

Does that include further optimisations to the modified functions, besides the proposed one? Or do I need to make a separate issue?

It's hard to say in general. Sometimes we have one issues to track multiple changes to a function or an area of code. Sometimes we're more atomic, and have a different issue to track each specific change. But whatever the case, it's important to have each issue clearly define the work you're proposing to do, and why you think it will be beneficial for users of CPython. If you want to optimise os.path, then you need to clearly state which optimisations you're planning in the issue before filing a PR. If you later see the potential for followup PRs with different optimisations, then yes, it would probably be best to open a new issue detailing the new optimisation, rather than reusing the old issue.

diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Barney Gale <barney.gale@gmail.com>
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants