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

8259947: (fs) Optimize UnixPath.encode implementation #2135

Closed
wants to merge 6 commits into from

Conversation

cl4es
Copy link
Member

@cl4es cl4es commented Jan 19, 2021

This patch improves UnixPath.encode by reusing JLA.getBytesNoRepl (which has fast-paths for common encoding) and avoiding a toCharArray call on the input by refactoring the normalizeNativePath code to operate on String. This might have a cost on files on Mac that need additional native normalization.

This removes another ThreadLocal and a source of SoftReferences. Together with the UTF-8 fast-path my UTF-8 encoded file system see substantial speed-ups in a trivial new File(str).toPath() microbenchmark.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8259947: (fs) Optimize UnixPath.encode implementation

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2135/head:pull/2135
$ git checkout pull/2135

@cl4es
Copy link
Member Author

cl4es commented Jan 19, 2021

Microbenchmark results, baseline:

Benchmark                 Mode  Cnt     Score    Error  Units
FileToPath.mix            avgt   15  1669.996 ± 76.308  ns/op
FileToPath.normalized     avgt   15   349.300 ± 16.851  ns/op
FileToPath.notNormalized  avgt   15   553.013 ± 28.736  ns/op
FileToPath.trailingSlash  avgt   15   415.107 ± 18.322  ns/op

Target:

Benchmark                 Mode  Cnt    Score    Error  Units
FileToPath.mix            avgt   15  887.191 ± 34.167  ns/op
FileToPath.normalized     avgt   15  132.653 ±  2.907  ns/op
FileToPath.notNormalized  avgt   15  333.678 ± 17.665  ns/op
FileToPath.trailingSlash  avgt   15  192.272 ±  7.644  ns/op

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 19, 2021

👋 Welcome back redestad! 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.

@openjdk
Copy link

openjdk bot commented Jan 19, 2021

@cl4es The following labels will be automatically applied to this pull request:

  • core-libs
  • nio

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

@openjdk openjdk bot added nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jan 19, 2021
@cl4es cl4es marked this pull request as ready for review Jan 19, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 19, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 19, 2021

Webrevs

Copy link
Member

@ChrisHegarty ChrisHegarty left a comment

I think that this looks good ( I had a similar thought when looking through this code recently, for a separate issue ).

@cl4es cl4es changed the title 8259947: Optimize UnixPath.encode 8259947: (fs) Optimize UnixPath.encode implementation Jan 19, 2021
@openjdk
Copy link

openjdk bot commented Jan 19, 2021

@cl4es 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:

8259947: (fs) Optimize UnixPath.encode implementation

Reviewed-by: chegar, shade, 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 1 new commit pushed to the master branch:

  • 52ed2aa: 8259786: initialize last parameter of getpwuid_r

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 19, 2021
public void mix(Blackhole bh) {
bh.consume(new File(normalFile).toPath());
bh.consume(new File(trailingSlash).toPath());
bh.consume(new File(root).toPath());
Copy link
Member

@shipilev shipilev Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No singular test for new File(root), but here it is in the mix anyway? What would probably be not comparable.

test/micro/org/openjdk/bench/java/io/FileToPath.java Outdated Show resolved Hide resolved
input = fs.normalizeNativePath(input);
try {
return JLA.getBytesNoRepl(input, Util.jnuEncoding());
} catch (CharacterCodingException cce) {
Copy link
Contributor

@AlanBateman AlanBateman Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The encode method pre-dates JLA.getBytesNoRepl and the recent optimisations so this is a good cleanup.

@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
@Warmup(time=2, iterations=5)
@Measurement(time=3, iterations=5)
@Fork(value=2, jvmArgs="-Xmx1g")
Copy link
Member

@shipilev shipilev Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this can be inherited from the enclosing benchmark.

Copy link
Member Author

@cl4es cl4es Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's only @State that doesn't inherit.

@Warmup(time=2, iterations=5)
@Measurement(time=3, iterations=5)
@Fork(value=2, jvmArgs="-Xmx1g")
public static class ToPath {
Copy link
Member

@shipilev shipilev Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really have to nest this test? It seems to gain nothing per se: the annotations and fields are still pretty much duplicated.

Copy link
Member Author

@cl4es cl4es Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no. Nesting was to pre-emptively decouple the micros, but it makes sense to squash them into one for now.

@cl4es
Copy link
Member Author

cl4es commented Jan 20, 2021

/integrate

@openjdk openjdk bot closed this Jan 20, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 20, 2021
@openjdk
Copy link

openjdk bot commented Jan 20, 2021

@cl4es Since your change was applied there have been 2 commits pushed to the master branch:

  • 69f90b5: 8259843: initialize dli_fname array before calling dll_address_to_library_name
  • 52ed2aa: 8259786: initialize last parameter of getpwuid_r

Your commit was automatically rebased without conflicts.

Pushed as commit 5891509.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated nio nio-dev@openjdk.org
4 participants