On Thu, 12 Mar 2026 09:27:56 GMT, Volkan Yazici <[email protected]> wrote:
> 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 > src/java.base/share/classes/java/net/URLStreamHandler.java line 488: > >> 486: >> 487: // Local variables used by concatenation >> 488: String protocol = u.getProtocol(), authSeparator, authority, >> path, querySeparator, query, refSeparator, ref; > > *Nit:* You may consider declaring the variables the first time they're > assigned. That would decrease the cognitive load on specialized > concatenations where only a subset of variables are needed. That said, > current form is okay too. Thank you Volkan for your feedback and testing! Feedback from Daniel suggested that the initial implementation was a bit bulky. I agree to this, so I marked the PR as draft until I could settle on a version which is denser while being correct and retaining performance characteristics. 108a8e8 is the best I can think of so far: It includes explicit locals for optionality, which by definition is subtly different for the authority component. Local variables are now effectively final and assigned by declaration as you suggested. Would be great if you could rerun your Linux x64 benchmark on this latest version. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30151#discussion_r2923871708 PR Review Comment: https://git.openjdk.org/jdk/pull/30151#discussion_r2917662264
