Skip to content

perf: optimize OpamString.split#6210

Merged
kit-ty-kate merged 2 commits intoocaml:masterfrom
kit-ty-kate:speedup-string-split
Dec 17, 2024
Merged

perf: optimize OpamString.split#6210
kit-ty-kate merged 2 commits intoocaml:masterfrom
kit-ty-kate:speedup-string-split

Conversation

@kit-ty-kate
Copy link
Copy Markdown
Member

@kit-ty-kate kit-ty-kate commented Sep 25, 2024 •
edited
Loading

this cuts the time spent on parsing pacman -Si significantly down

Commit by @c-cube split from #5757
Queued on #6212

@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha1 milestone Sep 25, 2024
@kit-ty-kate
Copy link
Copy Markdown
Member Author

kit-ty-kate commented Sep 25, 2024

After trying a couple of things, it looks like opam show dune shows the significant performance improvement (25% on my local Fedora), however Debian (the system used for benchmark) shows less improvement (6 or 7%). I'll add a benchmark nonetheless and see if it can be reproduced in a better controlled environment

@kit-ty-kate kit-ty-kate mentioned this pull request Sep 25, 2024
Merged
@kit-ty-kate kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Sep 25, 2024
@rjbou rjbou self-requested a review September 30, 2024 12:27
@kit-ty-kate kit-ty-kate marked this pull request as draft September 30, 2024 12:28
@kit-ty-kate
Copy link
Copy Markdown
Member Author

kit-ty-kate commented Dec 5, 2024

ok i should've just started with benchmarking just the function in isolation. It now shows a 45x speedup

@kit-ty-kate kit-ty-kate marked this pull request as ready for review December 5, 2024 06:27
rjbou
rjbou approved these changes Dec 16, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@rjbou rjbou left a comment

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 removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Dec 17, 2024
@c-cube @kit-ty-kate
perf: optimize OpamString.split
30827b0 
this cuts the time spent on parsing `pacman -Si` significantly down

Co-authored-by: Kate <kit-ty-kate@outlook.com>
@kit-ty-kate kit-ty-kate merged commit 6dfc1ec into ocaml:master Dec 17, 2024
@kit-ty-kate kit-ty-kate deleted the speedup-string-split branch December 17, 2024 17:07
@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Dec 17, 2024

60s to 1.2s, from bench

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

Assignees

No one assigned

Labels

AREA: PERFORMANCE

Projects

None yet

Milestone

2.4.0~alpha1

Development

Successfully merging this pull request may close these issues.

3 participants

@kit-ty-kate @rjbou @c-cube