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

perf: more efficient intersection #157

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Nov 29, 2023

I got nerds not by astral-sh#6. Which points out that intersection is slower than it needs to be when the basic operations on version are expensive. Here's what I've come up with.

The ideas are:

  1. Like that PR, avoid cloning till the end
  2. Instead of using next_if and eq keep track of which iter to advance directly.
  3. Because we know which iter end came from, and we know that for each iterator start < end, check for validity before determining start
  4. Don't redundantly check e > i
branch clone to reject compares to reject clones to accept comparisons to accept
dev 2 5 2 5
zanieb/#6 0 5 2 5
this 0 2 2 3

I tested this synthetically by making a version that increasing a global counter every time eq/cmp/clone/drop is called. This global counter goes up much (~30%) less after this PR.
Even on our normal benchmarks, where these operations are very cheap (a handful of CPU cycles), we see improvements (although only a few percent).

cc @konstin for real-world impact.
cc @baszalmstra mamba-org/resolvo#2 (comment) I don't know if these are improvements you'd be interested in include in your copy of this type.

@zanieb zanieb assigned zanieb and unassigned zanieb Nov 30, 2023
@zanieb zanieb self-requested a review November 30, 2023 05:21
@konstin
Copy link
Member

konstin commented Nov 30, 2023

I tried this branch on a pathological test case and it reduced the time from 10s to 3.6s!

@Eh2406
Copy link
Member Author

Eh2406 commented Nov 30, 2023

That is wonderful to hear! As soon as this gets a review, let's get this merged!

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, but may benefit from a bit of documentation?

@Eh2406
Copy link
Member Author

Eh2406 commented Nov 30, 2023

I rarely provide enough documentation or use good names for things. Always feel comfortable asking me to improve! Anything in particular you want me to clarify?

@zanieb
Copy link
Member

zanieb commented Nov 30, 2023

I think basically anywhere you're doing something unintuitive for performance reasons as outlined in your bullets in the pull request summary you'd benefit from adding a comment explaining what's going on e.g.

// We can conclude e > i and do not check again for perf

@Eh2406 Eh2406 changed the title refactor: more efficient intersection perf: more efficient intersection Nov 30, 2023
@Eh2406
Copy link
Member Author

Eh2406 commented Nov 30, 2023

Here's a small dissertation. Let me know if any of it made sense.

@Eh2406
Copy link
Member Author

Eh2406 commented Dec 1, 2023

If people think of improvements to the documentation I added, PR's are welcome. So I'm going to merge.

@Eh2406 Eh2406 added this pull request to the merge queue Dec 1, 2023
Merged via the queue into dev with commit 4c28c6c Dec 1, 2023
5 checks passed
@Eh2406 Eh2406 deleted the more_efficient_intersection branch December 1, 2023 16:32
zanieb added a commit to astral-sh/uv that referenced this pull request Dec 1, 2023
zanieb added a commit to astral-sh/uv that referenced this pull request Dec 1, 2023
zanieb added a commit to astral-sh/uv that referenced this pull request Dec 4, 2023
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.

3 participants