Improving copy performance since the packager depends on it now #2628
Conversation
| mkDirectory(to); | ||
| // directory | ||
| for (var child: sourceFS.directChildren(sourcePath)) { | ||
| var childPath = sourcePath + "/" + child; |
There was a problem hiding this comment.
Can we use a method here to, to hide the slash magic constant?
| var childPath = sourcePath + "/" + child; | ||
| var childTarget = URIUtil.getChildLocation(to, child); | ||
| if (sourceFS.isFile(childPath) || recursive) { | ||
| localCopy(sourceFS, sourcePath + "/" + child, childTarget, recursive , overwrite); |
There was a problem hiding this comment.
agreed, but the code already had this riddled in some places.
| /** | ||
| * Copy a file or directory where the source and target are both in the same scheme | ||
| */ | ||
| default void localCopy(ISourceLocation from, ISourceLocation to, boolean recursive, boolean overwrite) throws IOException { |
There was a problem hiding this comment.
"local" is confusing to me. Of course for file and memory this is local but the API does not enforce locality. How about "internal" as it is internal too the scheme implementation? Or something with "scheme" in it. "schemeCopy"? Or "internalCopy", or simply just "copy" and it's documented that a resolver can only copy to its own scheme.
There was a problem hiding this comment.
I'm for the last option. Just "copy". The context says a lot.
There was a problem hiding this comment.
I mean, it's only within the scheme, never across the scheme boundaries, so local to the resolver was my idea for the name.
jurgenvinju
left a comment
jurgenvinju
left a comment
There was a problem hiding this comment.
Looks like a great performance improvement. Curious about some micro benchmarks. I request one name change in the output interface. Otherwise it looks good. Nice that the memory resolver also chimes in 🦾 that makes this even more convincing.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2628 +/- ##
========================================
- Coverage 46% 46% -1%
+ Complexity 6666 6658 -8
========================================
Files 794 795 +1
Lines 65746 65857 +111
Branches 9852 9872 +20
========================================
- Hits 30646 30643 -3
- Misses 32739 32842 +103
- Partials 2361 2372 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
I've made a dedicated version of recursive file copy, as the packager would spend 95% of the time copying files, and our recursive copy implementation was less suited for this many files.