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
8266589: (fs) Improve performance of Files.copy() on macOS using copyfile(3) #3890
Conversation
|
Webrevs
|
} | ||
return COPYFILE_CONTINUE; | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of this fcopyfile but it seems to be a good match. At some point I think we will have to split up UnixCopyFile to make it easier to maintain the different implementations. That might be the opportunity to do more than COPYFILE_DATA, meaning we could get it to copy the meta data too.
I think we should try to re-format the callback a bit to make it easier to distinguish the conditions in the if-statement from the body. I probably should re-format the declaration too to get it a more more consistent/readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would be good to split it up but as part of a yet to be filed issue.
Would you please elaborate on reformatting the callback? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have been clearer. L58 splits the conditions on two lines with the second line indented 4 spaces so it looks like the 3rd condition is in the block. I think you should re-format it to make it easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be made into one line, no split, or three lines. Otherwise an additional if
would be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the re-examination of the callback means that COPYFILE_ERR is handled now. Good!
@bplb This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 90 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
/integrate |
@bplb Since your change was applied there have been 103 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit b5b3119. |
Please consider this request to use
fcopyfile(3)
to copy files viajava.nio.file.Files(Path,Path,CopyOption...)
on macOS. This change would improve the throughput ofFiles.copy()
as shown in these results:Copy via 8192 byte buffers
Copy via fcopyfile
The smallest size tested shows a small degradation in throughput, but this could be rectified if desired by adding an empirically determined size threshold under which user-space buffers would be used instead of
fcopyfile()
.A small change is also made to the Linux
sendfile()
portion to use the largest permittedcount
value if the copy is uninterruptible, i.e.,cancel == NULL
.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3890/head:pull/3890
$ git checkout pull/3890
Update a local copy of the PR:
$ git checkout pull/3890
$ git pull https://git.openjdk.java.net/jdk pull/3890/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3890
View PR using the GUI difftool:
$ git pr show -t 3890
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3890.diff