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

Cache find best candidate #9078

Merged
merged 2 commits into from
Oct 31, 2020

Conversation

xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Oct 31, 2020

Cache find_best_candidate results

This is possible because self.make_candidate_evaluator only depends
on:

  • the function arguments which are keys to the cache
  • self._target_python which never changes during a pip resolution
  • self._candidate_prefs which never changes during a pip resolution

On a fresh install, pip install <a package with ~ 100 dependencies>
runs on my machine in:

master (a0e34e9)

0m33.058s
0m34.105s
0m32.426s

This commit

0m15.860s
0m16.254s
0m15.910s

pip 20.2.4 - legacy resolver

0m15.145s
0m15.040s
0m15.152s

This is possible because self.make_candidate_evaluator only depends
on:
- the function arguments which are keys to the cache
- self._target_python which never changes during a pip resolution
- self._candidate_prefs which never changes during a pip resolution

On a fresh install, pip install <a package with ~ 100 dependencies>
runs on my machine in:

master (a0e34e9)
=======================

0m33.058s
0m34.105s
0m32.426s

This commit
===========

0m15.860s
0m16.254s
0m15.910s

pip 20.2.4 - legacy resolver
============================

0m15.145s
0m15.040s
0m15.152s
@McSinyx
Copy link
Contributor

McSinyx commented Oct 31, 2020

Does this mean that we can answer GH-8664 with We are there?

@uranusjr
Copy link
Member

I’m a bit concerned this may cause some subtle bugs someday if we do change one of the instance variables and forgot to consider the cache. Would it be better to extract the logic into a pure function (outside of the class) and cache that instead?

@pradyunsg
Copy link
Member

Ack. I agree w/ @uranusjr -- it'd be nice to make that a pure function (or a staticmethod on the same class) whose return value is cached. That'll ensure we're not caching function return values that are dependent on instance properties.

@xavfernandez
Copy link
Member Author

Does this mean that we can answer GH-8664 with We are there?

For my use case, We are ^^

Would it be better to extract the logic into a pure function (outside of the class) and cache that instead?

I had the same concern, but those same concerns would also apply to 1dd6d56 :)
That doesn't mean we should not refactor this part to make it clearer but IMHO this should not prevent this from being merged (given the user impact).

@pradyunsg pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Oct 31, 2020
@pradyunsg
Copy link
Member

Alrighty, I'm OK to defer the cleanups to a follow up PR. :)

@pradyunsg pradyunsg merged commit 2e11f31 into pypa:master Oct 31, 2020
@xavfernandez xavfernandez deleted the cache_find_best_candidate branch October 31, 2020 17:44
@pradyunsg
Copy link
Member

Anyone wanna file tracking issues for the cleanup follow-up PRs?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants