8353013: java.net.URI.create(String) may have low performance to scan the host/domain name from URI string when the hostname starts with number #24295

rk-kmr · 2025-03-28T15:19:46Z

scanByte throws NumberFormatException for URIs that start with numbers, e.g., https://11111111.x.y/
The current flow is parseIPv4Address → scanIPv4Address → scanByte. parseIPv4Address uses NumberFormatException for control flow, so it captures the exception, ignores it, and returns -1. This has been reported by AWS customer to cause low performance. Details: JDK-8353013 & aws/aws-sdk-java-v2#5933

This PR avoids NumberFormatException by skipping calls to Integer.parseInt if the number of digits in the octet is > 3.

I ran following tests.

make test-tier1
make test-tier2
make test TEST=jdk/java/net

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-8353013: java.net.URI.create(String) may have low performance to scan the host/domain name from URI string when the hostname starts with number (Bug - P4)

Reviewers

Xiaolong Peng (@pengxiaolong - Author) Review applies to 45fb1759
Michael McMahon (@Michael-Mc-Mahon - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24295

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-28T15:20:49Z

Hi @rk-kmr, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user rk-kmr" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-03-28T15:21:08Z

@rk-kmr 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:

8353013: java.net.URI.create(String) may have low performance to scan the host/domain name from URI string when the hostname starts with number

Reviewed-by: michaelm, xpeng

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

fe3bd5d: 8354667: [TESTBUG] AccessZeroNKlassHitsProtectionZone cds tests require cds
8c6b611: 8353832: Opensource FontClass, Selection and Icon tests
cfbbcd7: 8350595: jshell completion on arrays does not work for clone()
... and 321 more: https://git.openjdk.org/jdk/compare/1bd0ce1f51760d2e57e94b19b83d3ee0fa4aebcd...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@Michael-Mc-Mahon) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-03-28T15:22:00Z

@rk-kmr The following label will be automatically applied to this pull request:

net

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.

rk-kmr · 2025-03-28T15:22:33Z

/covered

bridgekeeper · 2025-03-28T15:23:16Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

pengxiaolong

Looks good to me, thanks.

src/java.base/share/classes/java/net/URI.java

myankelev · 2025-03-31T14:06:48Z

Could you please also add a test for this change/ add a bug id to the existing test if there already is one covering this functionality. Just in case

rk-kmr · 2025-03-31T16:40:40Z

Thanks! I have added unit test.

src/java.base/share/classes/java/net/URI.java

Co-authored-by: Johannes Graham <[email protected]>

mlbridge · 2025-04-11T18:41:38Z

Webrevs

AlanBateman · 2025-04-12T06:06:58Z

test/jdk/java/net/URI/Test.java

@@ -1786,6 +1787,28 @@ private static void b8272072() {
        }
    }

+    // 8353013 - java.net.URI.create(String) may have low performance to scan the host/domain name from
+    //           URI string when the hostname starts with number


This comment looks a bit out of place in a unit test. I think start with a JMH benchmark and change the comment in the unit test to make it clearer that it's providing more test coverage for cases where the authority component of a hierarchical URI has a host component that starts with a number.

Thanks! I have updated the comment and added a JMH benchmark.

## Before Benchmark (uri) Mode Cnt Score Error Units URIAuthorityParsingBenchmark.create https://98765432101.abc.xyz.com avgt 15 1495.273 ± 86.157 ns/op URIAuthorityParsingBenchmark.create https://ABCDEFGHIJK.abc.xyz.com avgt 15 363.048 ± 6.510 ns/op ## After Benchmark Mode Cnt Score Error Units Benchmark (uri) Mode Cnt Score Error Units URIAuthorityParsingBenchmark.create https://98765432101.abc.xyz.com avgt 15 377.741 ± 10.497 ns/op URIAuthorityParsingBenchmark.create https://ABCDEFGHIJK.abc.xyz.com avgt 15 334.215 ± 2.148 ns/op

Isn't is suspicious that non-number case got speedup too?

Not sure if we should consider a 30ns speed up in a single run of the benchmark on a single node as significant.

test/jdk/java/net/URI/Test.java

Michael-Mc-Mahon

This looks like a useful improvement. LGTM.

openjdk · 2025-04-16T10:31:18Z

⚠️ @rk-kmr the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8353013
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push

rk-kmr · 2025-04-16T10:44:06Z

/integrate

openjdk · 2025-04-16T10:44:43Z

@rk-kmr
Your change (at version 0164bca) is now ready to be sponsored by a Committer.

Michael-Mc-Mahon · 2025-04-16T12:42:08Z

We are going to run some more tests before sponsoring this PR.

Michael-Mc-Mahon · 2025-04-16T16:07:15Z

/sponsor

openjdk · 2025-04-16T16:08:32Z

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

c3de94c: 8352942: jdk/jfr/startupargs/TestMemoryOptions.java fails with 32-bit build
465c8e6: 8349721: Add aarch64 intrinsics for ML-KEM
1ad869f: 8322706: AnnotationTypeMismatchException in javac with annotation processing
... and 332 more: https://git.openjdk.org/jdk/compare/1bd0ce1f51760d2e57e94b19b83d3ee0fa4aebcd...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-16T16:08:46Z

@Michael-Mc-Mahon @rk-kmr Pushed as commit 84458ec.

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

Rohitash Kumar and others added 3 commits March 27, 2025 11:48

avoid parseInt if scanned digit count > 3

f9157d3

reuse scan method instead

cf217ee

Merge branch 'openjdk:master' into JDK-8353013

45fb175

bridgekeeper bot added the oca Needs verification of OCA signatory status label Mar 28, 2025

openjdk bot added the net [email protected] label Mar 28, 2025

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Mar 28, 2025

pengxiaolong approved these changes Mar 28, 2025

View reviewed changes

myankelev reviewed Mar 31, 2025

View reviewed changes

src/java.base/share/classes/java/net/URI.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/net/URI.java Show resolved Hide resolved

Rohitash Kumar added 3 commits March 31, 2025 15:10

Address PR comments

5e232a1

add unit test

1ae19b5

make test pass

f944774

j3graham reviewed Apr 3, 2025

View reviewed changes

src/java.base/share/classes/java/net/URI.java Outdated Show resolved Hide resolved

Update src/java.base/share/classes/java/net/URI.java

4a6c3d6

Co-authored-by: Johannes Graham <[email protected]>

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Apr 11, 2025

openjdk bot added the rfr Pull request is ready for review label Apr 11, 2025

AlanBateman reviewed Apr 12, 2025

View reviewed changes

dfuch reviewed Apr 14, 2025

View reviewed changes

test/jdk/java/net/URI/Test.java Show resolved Hide resolved

Rohitash Kumar added 3 commits April 14, 2025 17:18

Address PR comments

47016df

Add benchmark

07bff28

improve jmh benchmark

0164bca

Michael-Mc-Mahon approved these changes Apr 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 16, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 16, 2025

openjdk bot closed this Apr 16, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 16, 2025

8353013: java.net.URI.create(String) may have low performance to scan the host/domain name from URI string when the hostname starts with number #24295

8353013: java.net.URI.create(String) may have low performance to scan the host/domain name from URI string when the hostname starts with number #24295

Uh oh!

Conversation

rk-kmr commented Mar 28, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 28, 2025

Uh oh!

openjdk bot commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 28, 2025

Uh oh!

rk-kmr commented Mar 28, 2025

Uh oh!

bridgekeeper bot commented Mar 28, 2025

Uh oh!

pengxiaolong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

myankelev commented Mar 31, 2025

Uh oh!

rk-kmr commented Mar 31, 2025

Uh oh!

Uh oh!

mlbridge bot commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rk-kmr Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turbanoff Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

dfuch Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Apr 16, 2025

Uh oh!

rk-kmr commented Apr 16, 2025

Uh oh!

openjdk bot commented Apr 16, 2025

Uh oh!

Michael-Mc-Mahon commented Apr 16, 2025

Uh oh!

Michael-Mc-Mahon commented Apr 16, 2025

Uh oh!

openjdk bot commented Apr 16, 2025

Uh oh!

openjdk bot commented Apr 16, 2025

Uh oh!

Uh oh!

rk-kmr commented Mar 28, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 28, 2025 •

edited

Loading

mlbridge bot commented Apr 11, 2025 •

edited

Loading

AlanBateman Apr 12, 2025 •

edited

Loading

rk-kmr Apr 14, 2025 •

edited

Loading