-
Notifications
You must be signed in to change notification settings - Fork 351
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
Various speedups on tasks that do not require availability information (e.g. opam list --installed) #5317
Various speedups on tasks that do not require availability information (e.g. opam list --installed) #5317
Conversation
b5b0d66
to
9b2d813
Compare
9b2d813
to
38c333d
Compare
Bench shows a speed up on |
@@ -1346,7 +1346,7 @@ let dependencies_filter_to_formula_t ~build ~post st nv = | |||
OpamFilter.filter_formula ~default:true env | |||
|
|||
let dependencies_t st base_deps_compute deps_compute | |||
~depopts ~installed ?(unavailable=false) packages = | |||
~depopts ~installed ~unavailable packages = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this change speeds up dependencies computation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't, but as the dedicated commit says:
Make ?unavailable a non-optional argument to enforce speedups when availability information is not needed
it only enforces that the availability information is used purposefully and ensures that regression won't happen, as the default unavailable=false
makes it so that the availability information is used and thus the command will take significantly longer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I've read the commit message.
The optional argument unavailable
will anyway be used on purpose as when calling the function if we want unavailable packages we need to specify it and usually we don't want it. It won't change ow the function is used to have the argument optional or just labelled. As changes show, all omitted unavailable
argument were false, as when we need it, it is already to true.
I'm not convinced, but I won't request a change. It's worth mentioning it in the documentation of the function, that requesting only for available package will add an extra computation time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As changes show, all omitted
unavailable
argument were false, as when we need it, it is already to true.
I don't think that's an viable argument. We could've changed the behaviour of the commands that use this function if we had known and this holds for future change as well. Also i haven't checked that each commands actually require ~unavailable:false
. I simply used its previous default to avoid breaking the current behaviour and also because it's outside of the scope of this PR.
It's worth mentioning it in the documentation of the function, that requesting only for available package will add an extra computation time.
I agree. What do you think of e5277ca ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ffab04a
to
69d69ed
Compare
e5277ca
to
df46f93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
…not require availiblity information
…ailablity information is not needed
df46f93
to
4026447
Compare
Fixes #5314
Queued on #6149