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.

Sign up for GitHub

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

Jump to bottom

OpamFile: Make all writes atomic #5489

Merged
merged 1 commit into from
Aug 13, 2024
Merged

OpamFile: Make all writes atomic #5489

merged 1 commit into from
Aug 13, 2024

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Mar 23, 2023 •
edited
Loading

A continuation of #5476 (review)

Partially reverts 64a569f

@kit-ty-kate kit-ty-kate added this to the 2.3.0~alpha milestone Jul 9, 2024
@rjbou rjbou self-requested a review August 13, 2024 14:29
@rjbou
Copy link
Collaborator

rjbou commented Aug 13, 2024

Do we know if there is any performance regression?

@kit-ty-kate
Copy link
Member Author

Do we know if there is any performance regression?

All these files are written (in the worse case scenario) a couple times per opam calls and rename is a fairly fast system call, so no.

The actual worse case scenario is for opam admin add-constraint if you give something like ocaml<3 but even for that, i've just tried it and it's actually faster (2.9s vs 3.7s before)

rjbou
rjbou approved these changes Aug 13, 2024
View reviewed changes
Copy link
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.

On the code and idea, lgtm!
The commit and PR comment should mention that it is a partial revert of 64a569f, and with deprecation notice on OpamFilename.with_open_out_bin, it is good to merge!

src/core/opamFilename.mli Show resolved Hide resolved
@rjbou rjbou mentioned this pull request Aug 13, 2024
Closed
1 task
@kit-ty-kate kit-ty-kate mentioned this pull request Aug 13, 2024
Open
@kit-ty-kate kit-ty-kate merged commit f74e661 into ocaml:master Aug 13, 2024
29 checks passed
@kit-ty-kate kit-ty-kate deleted the atomic-OpamFile branch August 13, 2024 18:27
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: ENGINE STATE: READY TO MERGE
Projects
None yet
Milestone
2.3.0~alpha1
Development

Successfully merging this pull request may close these issues.
2 participants
@kit-ty-kate @rjbou