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

os: update mv fns, improve performance, add params struct to control overwrite behavior #20156

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Dec 11, 2023

The rename command that is used as first attempt by the os.mv function, fails when the target path exists, as rename doesn't overwrite. Then it falls back to using mv_by_cp which does overwrite (means the current default behavior of mv is to overwrite via mv_by_cp), but mv_by_cp is less performant.

Trying to use a consistent non-overwriting behavior would result in a breaking change.
Therefore, the PR leaves overwriting on by default - equivalent to the system command mv, but improves it's behavior to still use rename command when possible, and by allowing to disable overwriting via a params struct.

That the fallback towards overwriting by mv_by_cp happens quite often, becomes noticeable when disabling overwriting. E.g. CI runs:

outdated

A performance example: if the target directory already exists, falling back to os.mv_by_cp is already less than 100% performant when just wanting to move 2 1mb files.

const src = os.join_path_single(os.home_dir(), 'v_mv_test_src')
const dst = os.join_path_single(os.home_dir(), 'v_mv_test_dst')

fn create_tpaths() ! {
	os.mkdir_all(src)!
	// Create 2 1mb files.
	os.write_file(os.join_path(src, '1mb_file1.txt'), '0123456789'.repeat(100 * 1024))!
	os.write_file(os.join_path(src, '1mb_file2.txt'), '0123456789'.repeat(100 * 1024))!
}

sw := time.new_stopwatch()
for _ in 0 .. 100 {
	create_tpaths()!
	os.mv(src, dst)!
}
dump(sw.elapsed())

Current master:

[mv.v:38] sw.elapsed(): 480.736ms

PR changes:

[mv.v:38] sw.elapsed(): 189.677ms

The PR also does some other changes related to the os.mv command as incremental refactor.

🤖[deprecated] Generated by Copilot at 4cdda23

Added an overwrite option to os.mv and os.mv_by_cp functions and improved their behavior across platforms and partitions. Simplified and fixed some tools that use these functions. Updated the corresponding test function.

🤖[deprecated] Generated by Copilot at 4cdda23

  • Add overwrite parameter to os.mv and os.mv_by_cp functions to control whether the target should be overwritten if it exists (link,link,link)
  • Remove platform-specific code and fallback logic from vshader and vpm tools, since os.mv handles cross-partition and cross-platform cases consistently (link,link)
  • Fix error message in vbump tool to say 'move' instead of 'copy' (link)

@ttytm ttytm changed the title os: update mv fns, improve performance, add params struct to control … os: update mv fns, improve performance, add params struct to control overwrite behavior Dec 11, 2023
@ttytm ttytm marked this pull request as draft December 11, 2023 23:32
@ttytm ttytm closed this Dec 12, 2023
@ttytm
Copy link
Member Author

ttytm commented Dec 12, 2023

I was wrong about the performance boost.

When fixing os.mv to behave the same as the system command mv again, the performance is about the same. Maybe it would be more performant if the performance test (marked as outdated above) would be extended and done with a trashy HD, but I didn't want to go down that rabbit hole.

So the PR would be about adding an overwrite params struct, and cleaning up os.mv related code. mv would still overwrite via mv_by_cp.

@ttytm ttytm reopened this Dec 12, 2023
@ttytm ttytm marked this pull request as ready for review December 12, 2023 03:11
cmd/tools/vshader.v Outdated Show resolved Hide resolved
vlib/os/os.v Outdated Show resolved Hide resolved
Co-authored-by: Delyan Angelov <26967+spytheman@users.noreply.github.com>
@spytheman spytheman merged commit e5e26db into vlang:master Dec 13, 2023
48 checks passed
@ttytm ttytm deleted the os/mv-overwrite-params branch December 15, 2023 22:06
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.

None yet

2 participants