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

[Claes Redestad]

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!

/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('/');
>>
>>