RFR: 8242186: Reduce allocations in URLStreamHandler.parseURL for some cases (Was: Re: [NEW BUG] Small optimization in URLStreamHandler.parseURL)

[Chris Hegarty]

> On 6 Apr 2020, at 11:29, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> Hi,
> 
> (including  net-dev at .. since this is in java.net)
> 
> I intend to sponsor this trivial patch provided by Christoph:
> 
> diff -r 65b345254ca3 src/java.base/share/classes/java/net/URLStreamHandler.java
> --- a/src/java.base/share/classes/java/net/URLStreamHandler.java	Thu Apr 02 05:44:04 2020 +0000
> +++ b/src/java.base/share/classes/java/net/URLStreamHandler.java	Mon Apr 06 12:27:09 2020 +0200
> @@ -266,8 +266,8 @@
>                          spec.substring(start, limit);
> 
>             } else {
> -                String separator = (authority != null) ? "/" : "";
> -                path = separator + spec.substring(start, limit);
> +                path = spec.substring(start, limit);
> +                path = (authority != null) ? "/" + path : path;
>             }
>         } else if (queryOnly && path != null) {
>             int ind = path.lastIndexOf('/');
> 
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8242186
> 
> Let me know if there are any concerns, otherwise I'll push
> once tier1 testing comes back green. Thanks!

Thanks all for this nice micro-optimization. The changes look good to me.

-Chris.


> /Claes
> 
> On 2020-04-05 19:48, Claes Redestad wrote:
>> Hi Christoph,
>> looks good and trivial. I'll sponsor.
>> /Claes
>> On 2020-04-05 10:57, Christoph Dreis wrote:
>>> Hi,
>>> 
>>> I just noticed a small optimization opportunity in URLStreamHandler.parseURL for inputs like the following:
>>> 
>>>     new URL(new URL("file:"), "file:./path/to/some/local.properties");
>>> 
>>> In those cases there is no authority involved, which makes it possible to rewrite URLStreamHandler:L269-270:
>>>      String separator = (authority != null) ? "/" : "";
>>>      path = separator + spec.substring(start, limit);
>>> 
>>> into the following to avoid unnecessary string concatenations with an empty string:
>>> 
>>>       path = spec.substring(start, limit);
>>>       path = (authority != null) ? "/" + path : path;
>>> 
>>> I've written a small benchmark with two URLStreamHandler variants (one with the old parseURL and one with the new):
>>> 
>>> @BenchmarkMode(Mode.AverageTime)
>>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>>> public class MyBenchmark {
>>> 
>>>      @State(Scope.Benchmark)
>>>      public static class ThreadState {
>>> 
>>>          private URL context;
>>>          private URLStreamHandler newHandler = new NewURLStreamHandler();
>>>          private URLStreamHandler oldHandler = new OldURLStreamHandler();
>>>          private String spec = "file:./path/to/some/application.properties";
>>> 
>>>          public ThreadState() {
>>>              try {
>>>                  context = new URL("file:");
>>>              } catch (MalformedURLException ignored) {
>>> 
>>>              }
>>>          }
>>>      }
>>> 
>>>      @Benchmark
>>>      public URL testNew(ThreadState threadState) throws MalformedURLException {
>>>          return new URL(threadState.context, threadState.spec, threadState.newHandler);
>>>      }
>>> 
>>>      @Benchmark
>>>      public URL testOld(ThreadState threadState) throws MalformedURLException {
>>>          return new URL(threadState.context, threadState.spec, threadState.oldHandler);
>>>      }
>>> 
>>> }
>>> 
>>> The benchmark above yields the following results on my local machine:
>>> 
>>> Benchmark                                             Mode  Cnt     Score    Error   Units
>>> MyBenchmark.testNew                                   avgt   10   112,655 ±  3,494   ns/op
>>> MyBenchmark.testNew:·gc.alloc.rate                    avgt   10  1299,558 ± 41,238  MB/sec
>>> MyBenchmark.testNew:·gc.alloc.rate.norm               avgt   10   192,015 ±  0,003    B/op
>>> MyBenchmark.testNew:·gc.count                         avgt   10    98,000           counts
>>> MyBenchmark.testNew:·gc.time                          avgt   10   105,000               ms
>>> MyBenchmark.testOld                                   avgt   10   128,592 ±  1,183   ns/op
>>> MyBenchmark.testOld:·gc.alloc.rate                    avgt   10  1612,743 ± 15,792  MB/sec
>>> MyBenchmark.testOld:·gc.alloc.rate.norm               avgt   10   272,019 ±  0,001    B/op
>>> MyBenchmark.testOld:·gc.count                         avgt   10   110,000           counts
>>> MyBenchmark.testOld:·gc.time                          avgt   10   121,000               ms
>>> 
>>> 
>>> In case you think this is worthwhile I would need someone to sponsor the attached patch for me.
>>> I would highly appreciate that.
>>> 
>>> Cheers,
>>> Christoph
>>> 
>>> 
>>> ===== PATCH =====
>>> diff --git a/src/java.base/share/classes/java/net/URLStreamHandler.java b/src/java.base/share/classes/java/net/URLStreamHandler.java
>>> --- a/src/java.base/share/classes/java/net/URLStreamHandler.java
>>> +++ b/src/java.base/share/classes/java/net/URLStreamHandler.java
>>> @@ -266,8 +266,8 @@
>>>                            spec.substring(start, limit);
>>> 
>>>               } else {
>>> -                String separator = (authority != null) ? "/" : "";
>>> -                path = separator + spec.substring(start, limit);
>>> +                path = spec.substring(start, limit);
>>> +                path = (authority != null) ? "/" + path : path;
>>>               }
>>>           } else if (queryOnly && path != null) {
>>>               int ind = path.lastIndexOf('/');
>>> 
>>>