Skip to content

Speedup OpamVersionCompare by 25% - #5518

Merged
rjbou merged 3 commits into
ocaml:masterfrom
kit-ty-kate:speedup-OpamVersionCompare
Aug 8, 2024
Merged

Speedup OpamVersionCompare by 25%#5518
rjbou merged 3 commits into
ocaml:masterfrom
kit-ty-kate:speedup-OpamVersionCompare

Conversation

@kit-ty-kate

@kit-ty-kate kit-ty-kate commented Apr 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Doing so does, for example, speeds-up typical opam show <pkg> by 50%

I also tried a completely new implementation of OpamPackage.Version.compare that would have sped-up the comparison function by 30% instead but doing so made OpamPackage.Version.of_string slower, as i was actually parsing the string first and pre-processed it so the comparison function is faster, but it wasn't worth it in the end.

F.i.x.e.s. #5479 as it speeds-up switch loading time from 0.22s to 0.044s on my M1Pro.
Related to #4245 (cc @emillon)

Requires #6078

kit-ty-kate
kit-ty-kate commented Apr 21, 2023
View reviewed changes
Comment thread src/core/opamVersionCompare.ml
(* splits a version into (epoch,rest), without the separating ':'. The
* epoch is delimited by the leftmost occurrence of ':' in x, and is ""
* in case there is no ':' in x. *)
let extract_epoch x =

@kit-ty-kate kit-ty-kate Apr 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note for reviewers: This was removed because OpamPackage.Version.of_string never allowed :. This is only an unused remnant of the Dose3 implementation.

@talex5 talex5 Aug 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's good to know - getting rid of the remaining allocs should be a lot easier if we don't need to support epochs!

@rjbou rjbou added this to the 2.2.0~alpha milestone Apr 24, 2023
rjbou
rjbou approved these changes Apr 24, 2023
View reviewed changes

@rjbou rjbou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!!

@kit-ty-kate kit-ty-kate mentioned this pull request Apr 25, 2023
Merged
@kit-ty-kate kit-ty-kate added PR: QUEUED Pending pull request, waiting for other work to be merged or closed and removed STATE: READY TO MERGE labels Apr 25, 2023
@kit-ty-kate
kit-ty-kate force-pushed the speedup-OpamVersionCompare branch from 5e1b91b to ac8d472 Compare April 26, 2023 15:57
@kit-ty-kate kit-ty-kate added STATE: READY TO MERGE and removed PR: QUEUED Pending pull request, waiting for other work to be merged or closed STATE: READY TO MERGE labels Apr 26, 2023
@kit-ty-kate

kit-ty-kate commented Apr 26, 2023

Copy link
Copy Markdown
Member Author

Crap, my testing method for opam show and the misspelled command wasn't good.
What I was actually testing was the performance difference between OCaml 4.14.1 and 5.0.0 (how is that giving us 50% performance for free!?!)
Thanks to #5525 we're actually able to detect performance changes reliably so at least that's the good news.

The speedup for OpamVersionCompare is still present but it's not as significant for real-life commands sadly :(

@kit-ty-kate
kit-ty-kate force-pushed the speedup-OpamVersionCompare branch from ac8d472 to 7bbd13b Compare April 26, 2023 19:07
@rjbou rjbou removed this from the 2.2.0~alpha milestone Apr 27, 2023
@rjbou rjbou added PR: WIP Not for merge at this stage PR: NEEDS UPDATE labels May 8, 2023
@kit-ty-kate kit-ty-kate mentioned this pull request Jul 8, 2024
Merged
@kit-ty-kate
kit-ty-kate force-pushed the speedup-OpamVersionCompare branch from 7bbd13b to 71d09d0 Compare July 8, 2024 19:39
@kit-ty-kate kit-ty-kate added PR: QUEUED Pending pull request, waiting for other work to be merged or closed and removed PR: WIP Not for merge at this stage PR: NEEDS UPDATE labels Jul 8, 2024
@rjbou
rjbou self-requested a review July 9, 2024 09:01
@kit-ty-kate kit-ty-kate added this to the 2.3.0~alpha milestone Jul 9, 2024
@kit-ty-kate
kit-ty-kate force-pushed the speedup-OpamVersionCompare branch from c9546a9 to e7587ed Compare July 24, 2024 15:50
@kit-ty-kate kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Jul 24, 2024
@kit-ty-kate

kit-ty-kate commented Jul 24, 2024

Copy link
Copy Markdown
Member Author

The latest benchmark shows a 18% speedup, in line with the local tests. I believe this is ready to go.

@kit-ty-kate kit-ty-kate mentioned this pull request Aug 7, 2024
Merged
@kit-ty-kate
kit-ty-kate force-pushed the speedup-OpamVersionCompare branch from f30871b to 7760468 Compare August 7, 2024 17:32
@kit-ty-kate

kit-ty-kate commented Aug 7, 2024

Copy link
Copy Markdown
Member Author

After #6144 the PR now improves performance by 28%

rjbou
rjbou reviewed Aug 8, 2024
View reviewed changes

@rjbou rjbou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On the idea and the code itself, lgtm!
I'm wondering if we should update the header as now we are changing that piece of code. Previous version is still the same code that the one in dose (modulo formatting).
Also, can you update the main comment to highlight the change you made, the origin of the speedup.

Comment thread master_changes.md Outdated
Comment thread src/core/opamVersionCompare.ml
Comment thread src/core/opamVersionCompare.ml Outdated
Comment thread src/core/opamVersionCompare.ml Outdated
@kit-ty-kate
kit-ty-kate force-pushed the speedup-OpamVersionCompare branch from 7760468 to 510e56c Compare August 8, 2024 15:18
@kit-ty-kate
kit-ty-kate requested a review from rjbou August 8, 2024 15:20
rjbou
rjbou approved these changes Aug 8, 2024
View reviewed changes

@rjbou rjbou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!!

@rjbou
rjbou merged commit 600ac33 into ocaml:master Aug 8, 2024
@kit-ty-kate
kit-ty-kate deleted the speedup-OpamVersionCompare branch August 8, 2024 16:41
@kit-ty-kate kit-ty-kate changed the title Speedup OpamVersionCompare by 15% Speedup OpamVersionCompare by 25% Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rjbou rjbou rjbou approved these changes
+1 more reviewer
@talex5 talex5 talex5 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AREA: PERFORMANCE

Projects

None yet

Milestone

2.3.0~alpha1

Development

Successfully merging this pull request may close these issues.

3 participants

@kit-ty-kate @talex5 @rjbou