Perf improvement for Windows share #3095
Conversation
|
@rxxg Thanks for the PR! Looks like speedcopy doesn't work on *nix. Btw, how did the |
|
On my tests I'm getting native-level performance on file copies. |
Not sure I follow. You've provided examples of |
|
What I meant was that On the downside, I have no way of testing a *nix equivalent, hence the repeated Travis failures. I suppose the easiest thing would be to only activate the functionality on Windows. What do you think? |
|
@rxxg Those are amazing results! 🎉 Btw, is it even possible for us to trigger server-side copy? |
Yeah, that is not very nice. I see you've already submitted the patch to the upstream and the maintainer is pretty active, so looks like we will be able to use an officially released version soon 🤞 |
dvc/system.py
Outdated
| import shutil | ||
| import sys | ||
|
|
||
| if sys.version < (3, 8): |
There was a problem hiding this comment.
| if sys.version < (3, 8): | |
| if sys.version_info < (3, 8): |
There was a problem hiding this comment.
So 3.8 already uses sendfile? neat!
There was a problem hiding this comment.
Upstream speedcopy works for both Linux and Windows now. I will let you make the judgement as to whether to use speedcopy or pyfastcopy for the Linux side.
As you say, 3.8 should be faster, but they have taken a different approach on Windows (not using the win32 API). I don't know whether whether that is faster/more robust, probably worth revisiting when DVC is 3.8-compatible. It might be worth moving the version check higher up so that on 3.8 shutil always gets used.
|
@rxxg This looks amazing! 🎉 And will speedup *nix as well as windows, which is amazing^2. Thank you so much for contributing! 🙏 Please see some minor comments above. |
There are hardly any upstream tests, and the *nix code is very different from the Windows version, so I am slightly wary about the code quality of the |
I don't know ... I am on a journey of discovery 😉 I found this page which claims that server-side copy will be triggered by Windows explorer for compatible servers (so I assume that one or more of the win32 API points makes use of it behind the scenes) and for Linux for [EDIT] Not working with |
Why not remote login into your work machine instead? 🙂 |
Yeah, a bit worried about that myself. But our test suite is quite good. If we feel too sketchy about it, we could make this optimization an opt-in by introducing some config option. |
|
@rxxg Btw, please rebase. There has been some temporary issues with the test suite, which are now resolved. |
# Conflicts: # dvc/system.py
Done ... |
|
Woah, jumping late to the discussion but thanks a lot for the patch, @rxxg , those improvements are awesome! What's missing to merge this one? |
We're working off a patched copy of an upstream library, which is not great. I'll see if I can get the maintainer to do a minor version release. BTW, I messed up and deleted my fork, so I'm closing this PR and starting a new one (with the same code). |
|
Superseded by #3135. |
2 participants
❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Improves performance for Windows network shares (#3084).
Be aware, it uses a patched version of the external library speedcopy, so I don't expect you will want to pull as-is.
Also - my local tests (Windows machine) showed 17 failures, but no new ones from the base case.