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

Use patterns in server query when resolving version ranges #7673

Merged
merged 4 commits into from Sep 9, 2020

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Sep 8, 2020

Changelog: Fix: Use patterns in server query when resolving version ranges.
Docs: omit

Closes: #7672

After a poor performance in resolution of version ranges in Artifactory was reported (#5432), Conan introduced a workaround for the issue in #5433. With these changes now the query to Artifactory server was made removing the patterns:
http://localhost:8081/artifactory/api/conan/conan/v2/conans/search?q=cgal
instead of:
http://localhost:8081/artifactory/api/conan/conan/v2/conans/search?q=cgal%2F%2A%40user%2Fchannel&ignorecase=False
and then the ranges were resolved with all the packages on the client side.

This is causing a security gap as defining exclude patterns in Artifactory will not work as those patterns are not sent any more to the server as reported here: #7672. This branch reverts those changes.

Please, @Daniel-Roberts-Bose could you try this branch and check if the performance issues are still happening?

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@czoido czoido changed the title search with patterns back Use patterns in server query when resolving version ranges Sep 8, 2020
@Daniel-Roberts-Bose
Copy link

Daniel-Roberts-Bose commented Sep 8, 2020

So I ran a bunch of testing against this today and found that this change actually speeds up overall version resolution slightly. I ran a basic conan install on one of our largest dependency graphs (~150 requirements * 5 profiles + 100s of transitive dependencies) and overall the time using this change was about 30 seconds to 1 minute faster than without. Below are the results

Conan Version 1.28.0
Run 1: 5m49.515s
Run 2: 6m38.955s
Run 3: 5m35.843s
Run 4: 2m53.274s
Average: ~5m13.75s

Conan Version 1.30.0-dev (this change)
Run 1: 4m51.784s
Run 2: 3m39.915s
Run 3: 3m47.895s
Run 4: 3m37.795s
Average: ~3m58.5s

I am not entirely sure the reason for the speed up but its likely due to the fact that these lines have now been removed https://github.com/conan-io/conan/pull/7673/files#diff-9cbaa4b8bc79e66cc149585748868f20L155 when you are performing this list traversal thousands of times I guess it really can reduce time complexity a lot since this is an O(n*2) operation I believe. (Number of times called * size of return list from the remote)

As another totally separate side note. I think these times could be heavily reduced if there wasn't so much output to the console. Prints to standard out are very heavy on time to complete operations. If this thing just ran in the background and there was a way of disabling warning logs from conan these times would be vastly improved. The logs I am talking about here are the literally thousands of lines of "WARN: foo is being overriden by bar"

@czoido czoido added this to the 1.30 milestone Sep 9, 2020
@czoido
Copy link
Contributor Author

czoido commented Sep 9, 2020

Thanks a lot for testing @Daniel-Roberts-Bose
As the issue is not happening any more and the resolution times with these changes have improved we are marking this PR for the 1.30 milestone.
Regarding the output in the command line we are currently working in a redesign of the CLI but it will be for Conan v2.0 so probably all the output will be made with loggers and it will be possible to get rid of those warning.

@memsharded
Copy link
Member

memsharded commented Sep 9, 2020

Hi @Daniel-Roberts-Bose

Thanks very much for the testing. I think that it is also possible that Artifactory has learned to better index the necessary fields for the search since then, so the query including user/channel that used to be slower, now it is faster when these values are included.

So all looking great then, we can move forward and resolve #7672 (cc/ @maikelvdh) for next Conan 1.30 :) :)

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.

[bug] Search call is leaking package name details to Conan Center Index
3 participants