8293122: (fs) Use file cloning in macOS version of Files::copy method #10188

bplb · 2022-09-07T00:48:31Z

Use BSD system call clonefile if possible when using Files::copy on macOS.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8293122: (fs) Use file cloning in macOS version of Files::copy method

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10188/head:pull/10188
$ git checkout pull/10188

Update a local copy of the PR:
$ git checkout pull/10188
$ git pull https://git.openjdk.org/jdk pull/10188/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10188

View PR using the GUI difftool:
$ git pr show -t 10188

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10188.diff

bridgekeeper · 2022-09-07T00:50:03Z

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java

openjdk · 2022-09-07T00:52:29Z

@bplb The following label will be automatically applied to this pull request:

nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-09-07T00:55:37Z

Webrevs

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java

AlanBateman · 2022-09-07T15:14:18Z

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java

+        // Attempt to clone the source unless cloning is not supported,
+        // cancellation is not possible, or attributes are not to be copied
+        if (!cloneFileNotSupported && addressToPollForCancel == 0 &&
+            flags.copyPosixAttributes) {


This will need some thought as this means it will only be used when Files.copy is invoked with the COPY_ATTRIBUTES option.

clonefile appears to copy all attributes so if it is acceptable for that to occur if COPY_ATTRIBUTES is not set, then the flags.copyPosixAttributes check could be removed.

My opinion is that the improved throughput outweighs any detriment from copying attributes when COPY_ATTRIBUTES is not specified and so the flags.copyPosixAttributes check should be removed.

…veDispatcher

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java

…ure; remove getattrlist

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java

openjdk · 2022-09-12T16:44:33Z

@bplb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8293122: (fs) Use file cloning in macOS version of Files::copy method

Reviewed-by: alanb

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 25 new commits pushed to the master branch:

7f3250d: 8293787: Linux aarch64 build fails after 8292591
2a38791: 8292755: Non-default method in interface leads to a stack overflow in JShell
8351b30: 8293771: runtime/handshake/SystemMembarHandshakeTransitionTest.java fails if MEMBARRIER_CMD_QUERY is unsupported
91f9c0d: 8293774: Improve TraceOptoParse to dump the bytecode name
1169a15: 8291657: Javac assertion when compiling a method call with switch expression as argument
2baf251: 8293654: Improve SharedRuntime handling of continuation helper out-arguments
60f59a4: 8293660: Fix frame::sender_for_compiled_frame frame size assert
b3461c1: 8293680: PPC64BE build failure after JDK-8293344
7e02039: 8293647: Avoid unnecessary boxing in jdk.hotspot.agent
9039022: 8287394: AArch64: Remove cbuf parameter from far_call/far_jump/trampoline_call
... and 15 more: https://git.openjdk.org/jdk/compare/524af949370990df6f58a84ad2493eb1dcba2231...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

AlanBateman · 2022-09-13T11:05:56Z

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java

+    {
+        // Attempt to clone the source unless cancellation is not possible,
+        // or attributes are not to be copied
+        if (addressToPollForCancel == 0 && flags.copyPosixAttributes) {


It wouldn't be wrong to drop the check for copyPosixAttributes here, in which case they would be copied by default (the spec allows for that). The only issue is that someone might become dependent on this and then be surprised when they code behaves differently when run on another platform.

I'm curious if there is a way to test how clone behaves when it fails. What you have is okay although the unlink might only be needed when chown fails.

The only issue is that someone might become dependent on this and then be surprised when they code behaves differently when run on another platform.

This is a good reason to leave it as-is.

I'm curious if there is a way to test how clone behaves when it fails. What you have is okay although the unlink might only be needed when chown fails.

All the described error numbers except EIO are things which one would expect the call to verify before actually starting to clone, so for those cases it would be unexpected to find that the destination exists after the failure. Provoking an EIO failure might be difficult.

bplb · 2022-09-14T20:11:25Z

/integrate

openjdk · 2022-09-14T20:13:30Z

Going to push as commit a75ddb8.
Since your change was applied there have been 27 commits pushed to the master branch:

95c7c55: 8293402: hs-err file printer should reattempt stack trace printing if it fails
211fab8: 8291669: [REDO] Fix array range check hoisting for some scaled loop iv
7f3250d: 8293787: Linux aarch64 build fails after 8292591
2a38791: 8292755: Non-default method in interface leads to a stack overflow in JShell
8351b30: 8293771: runtime/handshake/SystemMembarHandshakeTransitionTest.java fails if MEMBARRIER_CMD_QUERY is unsupported
91f9c0d: 8293774: Improve TraceOptoParse to dump the bytecode name
1169a15: 8291657: Javac assertion when compiling a method call with switch expression as argument
2baf251: 8293654: Improve SharedRuntime handling of continuation helper out-arguments
60f59a4: 8293660: Fix frame::sender_for_compiled_frame frame size assert
b3461c1: 8293680: PPC64BE build failure after JDK-8293344
... and 17 more: https://git.openjdk.org/jdk/compare/524af949370990df6f58a84ad2493eb1dcba2231...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-14T20:13:47Z

@bplb Pushed as commit a75ddb8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8293122: (fs) Use file cloning in macOS version of Files::copy method

0084d91

openjdk bot added the rfr Pull request is ready for review label Sep 7, 2022

bplb commented Sep 7, 2022

View reviewed changes

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java Outdated Show resolved Hide resolved

openjdk bot added the nio nio-dev@openjdk.org label Sep 7, 2022

AlanBateman reviewed Sep 7, 2022

View reviewed changes

8293122: Revert BsdFileStore; move volume capability check to BsdNati…

a7917d2

…veDispatcher

AlanBateman reviewed Sep 9, 2022

View reviewed changes

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java Outdated Show resolved Hide resolved

8293122: remove volatile cloneFileNotSupported; unlink target on fail…

f51cd18

…ure; remove getattrlist

AlanBateman reviewed Sep 10, 2022

View reviewed changes

src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java Outdated Show resolved Hide resolved

8293122: Move rollback unlink to copyFile method

96db856

AlanBateman approved these changes Sep 12, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2022

AlanBateman reviewed Sep 13, 2022

View reviewed changes

Merge

afc9c67

AlanBateman approved these changes Sep 14, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 14, 2022

openjdk bot closed this Sep 14, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 14, 2022

bplb deleted the Files-clonefile-8293122 branch September 14, 2022 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8293122: (fs) Use file cloning in macOS version of Files::copy method #10188

8293122: (fs) Use file cloning in macOS version of Files::copy method #10188

bplb commented Sep 7, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 7, 2022

openjdk bot commented Sep 7, 2022

mlbridge bot commented Sep 7, 2022 •

edited

Loading

AlanBateman Sep 7, 2022

bplb Sep 7, 2022

bplb Sep 9, 2022

openjdk bot commented Sep 12, 2022 •

edited

Loading

AlanBateman Sep 13, 2022 •

edited

Loading

bplb Sep 13, 2022

bplb commented Sep 14, 2022

openjdk bot commented Sep 14, 2022

openjdk bot commented Sep 14, 2022

8293122: (fs) Use file cloning in macOS version of Files::copy method #10188

8293122: (fs) Use file cloning in macOS version of Files::copy method #10188

Conversation

bplb commented Sep 7, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 7, 2022

openjdk bot commented Sep 7, 2022

mlbridge bot commented Sep 7, 2022 • edited Loading

Webrevs

AlanBateman Sep 7, 2022

Choose a reason for hiding this comment

bplb Sep 7, 2022

Choose a reason for hiding this comment

bplb Sep 9, 2022

Choose a reason for hiding this comment

openjdk bot commented Sep 12, 2022 • edited Loading

AlanBateman Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

bplb Sep 13, 2022

Choose a reason for hiding this comment

bplb commented Sep 14, 2022

openjdk bot commented Sep 14, 2022

openjdk bot commented Sep 14, 2022

bplb commented Sep 7, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 7, 2022 •

edited

Loading

openjdk bot commented Sep 12, 2022 •

edited

Loading

AlanBateman Sep 13, 2022 •

edited

Loading