On Thu, 12 Mar 2026 11:08:03 GMT, Eirik Bjørsnøs <[email protected]> wrote:
>> src/java.base/share/classes/java/net/URLStreamHandler.java line 483:
>>
>>> 481: * @return a string representation of the {@code URL} argument.
>>> 482: */
>>> 483: protected String toExternalForm(URL u) {
>>
>> I've tried to outsmart @eirbjo, and failed beautifully:
>>
>> 1. In the original method (5e588085e9c), replaced the conditional string
>> concatenation with an explicit `StringBuilder` that is populated using `if`
>> blocks:
>>
>> var sb = new StringBuilder(u.getProtocol()).append(':');
>> var authority = u.getAuthority();
>> if (authority != null && !authority.isEmpty) {
>> sb.append("//").append(authority);
>> }
>> // ...
>>
>> This hardly yielded any benefit. I concluded that fast path is indeed
>> helping.
>>
>> 2. In the most recent commit (4a11d5ed25f), in the slow path (i.e., not
>> specialized), replaced locals with `StringBuilder`+`if` stunt I've tried to
>> pull above. This resulted in performance regression. I concluded that
>> indified string concatenation, where lengths of the parts to be concatenated
>> are known in advance, does deliver.
>>
>> I suggest adding a comment block to the beginning of this method explaining
>> why it is fast. Inspirational material:
>>
>> // This method exercises following optimizations:
>> //
>> // 1. Fast paths are introduced for common cases
>> //
>> // 2. All string concatenations are intentionally performed without any
>> // conditionals to allow indified string concatenation to kick in.
>>
>> In particular, I find the 2nd warning important. IMHO, it is very likely
>> someone can accidentally update an existing `foo + bar` with `foo + (x ? "a"
>> : "b") + bar`, and we're back to square one.
>
>> I suggest adding a comment block to the beginning of this method explaining
>> why it is fast.
>
> I'll be completely honest here and admit that my understanding of _why_ this
> is fast is a bit shallow:
>
> * I added the fast path(s) because of my intuition that URLs without query or
> ref are common. However, removing the fast path also seems to make slow paths
> slower. C2 magic?
> * Initially I also thought about indified string concats, but then javac
> generates plain StringBuilder.append calls for this method, probably because
> this is in `java.base` and we want to avoid any issues in early bootstrap. Ind
> * I observe that consecutive StringBuilder::append calls of locals without
> any interleaving conditional code is faster. Again, my guess is there is some
> C2 optimization at play which does not kick in with the interleaving
> conditionals.
> * A single concatenation of more components is faster than combining the
> result of other concatenations
>
>
>>Inspirational material:
>
> What do you think of the comment I just pushed:
>
>
> // Optimizations in this method are based on the following observations:
> // 1: The query and ref components are commonly empty and can be specialized
> // 2: String concatenation is faster when locals are concatenated with no
> interleaving code
> // 3: A single concatenation is faster than combining results of other
> concatenations
> I've tried to outsmart @eirbjo, and failed beautifully:
>
> 1. In the original method
> ([5e58808](https://github.com/openjdk/jdk/commit/5e588085e9c333e9ad8e48dd7c60e665d30e278d)),
> replaced the conditional string concatenation with an explicit
> `StringBuilder` that is populated using `if` blocks:
> ```
> var sb = new StringBuilder(u.getProtocol()).append(':');
> var authority = u.getAuthority();
> if (authority != null && !authority.isEmpty) {
> sb.append("//").append(authority);
> }
> // ...
Since javac actually generates StringBuilder::append calls here, we can get
equal bytecode and performance by using StringBuilder directly. But for good
performance, the same rule of no interleaving code applies, so you'll need to
do:
```java
return new StringBuilder().append(u.getProtocol()).append("://")..
In fact it seems we can get _slightly better_ performance by initializing the
StringBuilder with the always-present protcol component:
```java
return new StringBuilder(u.getProtocol()).append("://")..
But the benefits are too marginal to deserve the code bloat.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30151#discussion_r2923904513
