RFR: 8193034: Optimize URL.toExternalForm

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8193034: Optimize URL.toExternalForm

Martin Buchholz-3
Reply | Threaded
Open this post in threaded view
|

RE: 8193034: Optimize URL.toExternalForm

Langer, Christoph

Hi Martin,

 

I think the new code is more compact and readable which makes it nice. I reviewed it to check that the result is still the same as before. However, I can’t assess if it is acceptable from a performance point of view and would defer this assessment to some other reviewer. But apart from performance constraints the change seems good.

 

Best regards

Christoph

 

From: net-dev [mailto:[hidden email]] On Behalf Of Martin Buchholz
Sent: Dienstag, 5. Dezember 2017 05:01
To: net-dev <[hidden email]>
Subject: RFR: 8193034: Optimize URL.toExternalForm

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193034: Optimize URL.toExternalForm

Chris Hegarty
In reply to this post by Martin Buchholz-3

> On 5 Dec 2017, at 04:01, Martin Buchholz <[hidden email]> wrote:
>
> http://cr.openjdk.java.net/~martin/webrevs/jdk/URL.toExternalForm/

Since the motivation for this change is performance, do you have any
performance numbers / benchmarks that you can share?

-Chris.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193034: Optimize URL.toExternalForm

Alan Bateman
In reply to this post by Martin Buchholz-3
On 05/12/2017 04:01, Martin Buchholz wrote:
The style is interesting but I don't see anything obvious wrong.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193034: Optimize URL.toExternalForm

Martin Buchholz-3
In reply to this post by Chris Hegarty
There's now a Martin-style benchmark at 
that suggests the code is ~ 25% faster with default JVM flags (C2) but ~ 25% slower with C1, as you might expect with multiple String concatenation.  I think we want to optimize for the default and assume that multiple String concatenation does in fact get optimized.
I'm not planning to check in the benchmark code.

On Tue, Dec 5, 2017 at 2:10 AM, Chris Hegarty <[hidden email]> wrote:

> On 5 Dec 2017, at 04:01, Martin Buchholz <[hidden email]> wrote:
>
> http://cr.openjdk.java.net/~martin/webrevs/jdk/URL.toExternalForm/

Since the motivation for this change is performance, do you have any
performance numbers / benchmarks that you can share?

-Chris.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193034: Optimize URL.toExternalForm

Chris Hegarty
Martin,

I think your changes are good. Thanks for sharing your benchmark
and results.

-Chris.

On 11/12/17 17:24, Martin Buchholz wrote:

> There's now a Martin-style benchmark at
> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLMicroBenchmark/
> that suggests the code is ~ 25% faster with default JVM flags (C2) but ~
> 25% slower with C1, as you might expect with multiple String
> concatenation.  I think we want to optimize for the default and assume
> that multiple String concatenation does in fact get optimized.
> I'm not planning to check in the benchmark code.
>
> On Tue, Dec 5, 2017 at 2:10 AM, Chris Hegarty <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>
>      > On 5 Dec 2017, at 04:01, Martin Buchholz <[hidden email]
>     <mailto:[hidden email]>> wrote:
>      >
>      >
>     http://cr.openjdk.java.net/~martin/webrevs/jdk/URL.toExternalForm/
>     <http://cr.openjdk.java.net/~martin/webrevs/jdk/URL.toExternalForm/>
>
>     Since the motivation for this change is performance, do you have any
>     performance numbers / benchmarks that you can share?
>
>     -Chris.
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193034: Optimize URL.toExternalForm

Alan Bateman
In reply to this post by Martin Buchholz-3
On 11/12/2017 17:24, Martin Buchholz wrote:
> There's now a Martin-style benchmark at
> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLMicroBenchmark/ 
> <http://cr.openjdk.java.net/%7Emartin/webrevs/jdk/URLMicroBenchmark/>
> that suggests the code is ~ 25% faster with default JVM flags (C2) but
> ~ 25% slower with C1, as you might expect with multiple String
> concatenation.  I think we want to optimize for the default and assume
> that multiple String concatenation does in fact get optimized.
> I'm not planning to check in the benchmark code.
>
Good to confirm the improvement, I think the change is good to push.

-Alan