On Mon, 31 Mar 2025 12:40:27 GMT, Mikhail Yankelevich <myankelev...@openjdk.org> wrote:
>> `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](https://bugs.openjdk.org/browse/JDK-8353013) & >> https://github.com/aws/aws-sdk-java-v2/issues/5933 >> >> This PR avoids NumberFormatException by skipping calls to `Integer.parseInt` >> if the number of digits in the octet is > 3. >> >> I benchmarked on local machine for potential regressions. >> https://gist.github.com/rk-kmr/cb1a3d59225c17b180a29cc125ebf887 >> >> >> Benchmark Mode Cnt Score >> Error Units >> URIBenchMark.newImplWithNormalUrl thrpt 10 0.004 ± >> 0.001 ops/ns >> URIBenchMark.newImplWithNumberlUrl thrpt 10 0.004 ± >> 0.001 ops/ns >> URIBenchMark.oldImplWithNormalUrl thrpt 10 0.004 ± >> 0.001 ops/ns >> URIBenchMark.oldImplWithNumUrl thrpt 10 0.001 ± >> 0.001 ops/ns >> URIBenchMark.newImplWithNormalUrl avgt 10 236.762 ± >> 8.700 ns/op >> URIBenchMark.newImplWithNumberlUrl avgt 10 264.017 ± >> 7.274 ns/op >> URIBenchMark.oldImplWithNormalUrl avgt 10 233.853 ± >> 6.539 ns/op >> URIBenchMark.oldImplWithNumUrl avgt 10 1183.572 ± >> 29.242 ns/op >> >> >> I ran following tests. >> >> make test-tier1 >> make test-tier2 >> make test TEST=jdk/java/net > > src/java.base/share/classes/java/net/URI.java line 3429: > >> 3427: int q = scan(p, n, L_DIGIT, H_DIGIT); >> 3428: if (q <= p) return q; >> 3429: > > Nit: Copyright year Updated. > src/java.base/share/classes/java/net/URI.java line 3435: > >> 3433: >> 3434: // Calculate the number of significant digits (after >> leading zeros) >> 3435: int significantDigits = q - i; > > Nit: Do you think it might be more clear if this is named something like > `significantDigitsNum` or similar? No strong feelings, just confused me a > little when I was reading this. I can see it leading to confusion. I have changed it to `significantDigitsNum` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24295#discussion_r2021131607 PR Review Comment: https://git.openjdk.org/jdk/pull/24295#discussion_r2021131283
