Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Alan Bateman]

On 28/01/2016 23:47, Claes Redestad wrote:


Sure, as this is package-private API a short description and reference 
to the URI::toURL method whose specification we adhere to should suffice:


http://cr.openjdk.java.net/~redestad/8147462/webrev.06/


This looks good - thanks!

-Alan

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Claes Redestad]

On 2016-01-28 20:56, Alan Bateman wrote:


This looks okay. I think it would be good to put a javadoc comment on 
this method, also maybe move it to below all the constructors rather 
than in the middle.


-Alan.


Sure, as this is package-private API a short description and reference 
to the URI::toURL method whose specification we adhere to should suffice:


http://cr.openjdk.java.net/~redestad/8147462/webrev.06/

Thanks!

/Claes

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Alan Bateman]

This looks okay. I think it would be good to put a javadoc comment on 
this method, also maybe move it to below all the constructors rather 
than in the middle.


-Alan.


On 28/01/2016 16:38, Claes Redestad wrote:



On 2016-01-28 16:52, Chris Hegarty wrote:

...

This looks fine Claes.


Thanks, Chris!



Maybe just a comment on the fact that character based comparison is
being used for perf reasons.

In an older version of the webrev, there was an updated to an 
existing test,

test/java/net/URI/URItoURLTest.java. I assume this was just mistakenly
omitted from the latest version?


Fixed test and added a comment:

http://cr.openjdk.java.net/~redestad/8147462/webrev.05/

Thanks!

/Claes

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Chris Hegarty]

On 28 Jan 2016, at 16:38, Claes Redestad  wrote:

> On 2016-01-28 16:52, Chris Hegarty wrote:
>>> ...
>> This looks fine Claes.
> 
> Thanks, Chris!
> 
>> 
>> Maybe just a comment on the fact that character based comparison is
>> being used for perf reasons.
>> 
>> In an older version of the webrev, there was an updated to an existing test,
>> test/java/net/URI/URItoURLTest.java. I assume this was just mistakenly
>> omitted from the latest version?
> 
> Fixed test and added a comment:
> 
> http://cr.openjdk.java.net/~redestad/8147462/webrev.05/

Reviewed.

-Chris.

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Claes Redestad]

On 2016-01-28 16:52, Chris Hegarty wrote:

...

This looks fine Claes.


Thanks, Chris!



Maybe just a comment on the fact that character based comparison is
being used for perf reasons.

In an older version of the webrev, there was an updated to an existing test,
test/java/net/URI/URItoURLTest.java. I assume this was just mistakenly
omitted from the latest version?


Fixed test and added a comment:

http://cr.openjdk.java.net/~redestad/8147462/webrev.05/

Thanks!

/Claes

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Chris Hegarty]

On 27 Jan 2016, at 17:24, Claes Redestad  wrote:

> 
> On 2016-01-18 17:20, Claes Redestad wrote:
>> 
>> The ability for URLStreamHandler implementations to override the parseURL 
>> method seem to prevent this improvement unless we only do this for a subset 
>> of known, well-behaved URLStreamHandlers, which likely defeat the 
>> optimization by adding complexity. 
> 
> Right, the fast path can only be safely used for the non-overrideable 
> handlers (jrt and file), but turns out we can make that work without 
> penalizing other cases:
> 
> http://cr.openjdk.java.net/~redestad/8147462/webrev.04/

This looks fine Claes.

Maybe just a comment on the fact that character based comparison is
being used for perf reasons.

In an older version of the webrev, there was an updated to an existing test, 
test/java/net/URI/URItoURLTest.java. I assume this was just mistakenly
omitted from the latest version?

-Chris.

> Relevant micros[1] show that this brings a benefit to file/jrt, even when 
> mixed with slow path URIs, while micros only hitting slow path 
> (newHttpURIToURL, opaqueURIToURL) doesn't regress:
> 
> Before:
> Benchmark Mode  CntScoreError Units
> URIBench.mixedURIToURLavgt   30  463.748 ± 14.445 ns/op
> URIBench.newHttpURIToURL  avgt   30  441.497 ± 20.173 ns/op
> URIBench.newURIToURL  avgt   30  227.106 ±  9.055 ns/op
> URIBench.opaqueURIToURL   avgt   30  320.904 ± 13.232 ns/op
> 
> Patched:
> Benchmark Mode  CntScoreError Units
> URIBench.mixedURIToURLavgt   30  441.773 ± 16.530 ns/op
> URIBench.newHttpURIToURL  avgt   30  433.946 ± 18.569 ns/op
> URIBench.newURIToURL  avgt   30  147.379 ±  6.349 ns/op
> URIBench.opaqueURIToURL   avgt   30  316.632 ± 12.940 ns/op
> 
> /Claes
> 
> [1] http://cr.openjdk.java.net/~redestad/8147462/URIBench.java

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Claes Redestad]

On 2016-01-18 17:20, Claes Redestad wrote:


The ability for URLStreamHandler implementations to override the 
parseURL method seem to prevent this improvement unless we only do 
this for a subset of known, well-behaved URLStreamHandlers, which 
likely defeat the optimization by adding complexity. 


Right, the fast path can only be safely used for the non-overrideable 
handlers (jrt and file), but turns out we can make that work without 
penalizing other cases:


http://cr.openjdk.java.net/~redestad/8147462/webrev.04/

Relevant micros[1] show that this brings a benefit to file/jrt, even 
when mixed with slow path URIs, while micros only hitting slow path 
(newHttpURIToURL, opaqueURIToURL) doesn't regress:


Before:
Benchmark Mode  CntScoreError Units
URIBench.mixedURIToURLavgt   30  463.748 ± 14.445 ns/op
URIBench.newHttpURIToURL  avgt   30  441.497 ± 20.173 ns/op
URIBench.newURIToURL  avgt   30  227.106 ±  9.055 ns/op
URIBench.opaqueURIToURL   avgt   30  320.904 ± 13.232 ns/op

Patched:
Benchmark Mode  CntScoreError Units
URIBench.mixedURIToURLavgt   30  441.773 ± 16.530 ns/op
URIBench.newHttpURIToURL  avgt   30  433.946 ± 18.569 ns/op
URIBench.newURIToURL  avgt   30  147.379 ±  6.349 ns/op
URIBench.opaqueURIToURL   avgt   30  316.632 ± 12.940 ns/op

/Claes

[1] http://cr.openjdk.java.net/~redestad/8147462/URIBench.java

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Alan Bateman]

On 18/01/2016 19:26, Chris Hegarty wrote:


On 18 Jan 2016, at 16:20, Claes Redestad > wrote:


On 2016-01-18 15:34, Chris Hegarty wrote:

On 18/01/16 14:09, Alan Bateman wrote:


On 17/01/2016 15:30, Claes Redestad wrote:

Hi,

please review this patch which might make URI.toURL more efficient

Webrev: http://cr.openjdk.java.net/~redestad/8147462 


Bug: https://bugs.openjdk.java.net/browse/JDK-8147462

This patch exploits the fact that URI will apply the same validation
to the URI/URL specification for any valid non-opaque URL, thus it's
safe to use the component-based URL constructor. Also, URIs
representing malformed URLs will throw an exception as specified both
before and after. A number of simple test cases to capture and
document this was added to the existing java/net/URI/URItoURL 
jtreg test.

I think the change to URI looks okay but it would be good to include a
brief comment as otherwise it will be difficult for future maintainers
to understand.


+1.  For a long time we have been suggesting that anyone requiring
a URL should retrieve it from URI::toURL, so it is good to have a
fast(er) common path.

The handling of a null/empty host is a little esoteric, so a comment
would be good.


How about this?

http://cr.openjdk.java.net/~redestad/8147462/webrev.02 



Looks good Claes.


This looks okay to me too.

-Alan

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Chris Hegarty]

> On 18 Jan 2016, at 16:20, Claes Redestad  wrote:
> 
> On 2016-01-18 15:34, Chris Hegarty wrote:
>> On 18/01/16 14:09, Alan Bateman wrote:
>>> 
>>> On 17/01/2016 15:30, Claes Redestad wrote:
 Hi,
 
 please review this patch which might make URI.toURL more efficient
 
 Webrev: http://cr.openjdk.java.net/~redestad/8147462
 Bug: https://bugs.openjdk.java.net/browse/JDK-8147462
 
 This patch exploits the fact that URI will apply the same validation
 to the URI/URL specification for any valid non-opaque URL, thus it's
 safe to use the component-based URL constructor. Also, URIs
 representing malformed URLs will throw an exception as specified both
 before and after. A number of simple test cases to capture and
 document this was added to the existing java/net/URI/URItoURL jtreg test.
>>> I think the change to URI looks okay but it would be good to include a
>>> brief comment as otherwise it will be difficult for future maintainers
>>> to understand.
>> 
>> +1.  For a long time we have been suggesting that anyone requiring
>> a URL should retrieve it from URI::toURL, so it is good to have a
>> fast(er) common path.
>> 
>> The handling of a null/empty host is a little esoteric, so a comment
>> would be good.
> 
> How about this?
> 
> http://cr.openjdk.java.net/~redestad/8147462/webrev.02 
> 

Looks good Claes.

-Chris.



>> I do have a little discomfort about this change, since the toURL spec
>> specifically says it is "equivalent to evaluating the expression new 
>> URL(this.toString())". I wonder if your tests should cover this too,
>> i.e. the resulting URLs from toURL and URL(this.toString()) are equal?
> 
> The test already does this. I've just added cases that exercise some 
> interesting cases like URIs containing /./ and quoted characters.
> 
> I restructured the exceptional tests slightly to differentiate the cases 
> where both toURL and new URL throw MalformedURLException from cases where 
> toURL throws IAE. It makes the test slightly more verbose but arguably a lot 
> simpler.
> 
> However: the javadoc spec for URL(String) seems incomplete, and I might have 
> stumbled into a trap...
> 
> * @exception  MalformedURLException  if no protocol is specified, or an
> *   unknown protocol is found, or {@code spec} is {@code 
> null}.
> 
> Since this method delegates parsing to the URLStreamHandler associated with 
> the protocol it might be overriden to throw exceptions (that will be wrapped 
> in MalformedURLException) for a variety of reasons.
> 
> The ability for URLStreamHandler implementations to override the parseURL 
> method seem to prevent this improvement unless we only do this for a subset 
> of known, well-behaved URLStreamHandlers, which likely defeat the 
> optimization by adding complexity.
> 
> Thanks!
> 
> /Claes

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Claes Redestad]

On 2016-01-18 15:34, Chris Hegarty wrote:

On 18/01/16 14:09, Alan Bateman wrote:


On 17/01/2016 15:30, Claes Redestad wrote:

Hi,

please review this patch which might make URI.toURL more efficient

Webrev: http://cr.openjdk.java.net/~redestad/8147462
Bug: https://bugs.openjdk.java.net/browse/JDK-8147462

This patch exploits the fact that URI will apply the same validation
to the URI/URL specification for any valid non-opaque URL, thus it's
safe to use the component-based URL constructor. Also, URIs
representing malformed URLs will throw an exception as specified both
before and after. A number of simple test cases to capture and
document this was added to the existing java/net/URI/URItoURL jtreg 
test.

I think the change to URI looks okay but it would be good to include a
brief comment as otherwise it will be difficult for future maintainers
to understand.


+1.  For a long time we have been suggesting that anyone requiring
a URL should retrieve it from URI::toURL, so it is good to have a
fast(er) common path.

The handling of a null/empty host is a little esoteric, so a comment
would be good.


How about this?

http://cr.openjdk.java.net/~redestad/8147462/webrev.02



I do have a little discomfort about this change, since the toURL spec
specifically says it is "equivalent to evaluating the expression new 
URL(this.toString())". I wonder if your tests should cover this too,

i.e. the resulting URLs from toURL and URL(this.toString()) are equal?


The test already does this. I've just added cases that exercise some 
interesting cases like URIs containing /./ and quoted characters.


I restructured the exceptional tests slightly to differentiate the cases 
where both toURL and new URL throw MalformedURLException from cases 
where toURL throws IAE. It makes the test slightly more verbose but 
arguably a lot simpler.


However: the javadoc spec for URL(String) seems incomplete, and I might 
have stumbled into a trap...


 * @exception  MalformedURLException  if no protocol is specified, 
or an
 *   unknown protocol is found, or {@code spec} is 
{@code null}.


Since this method delegates parsing to the URLStreamHandler associated 
with the protocol it might be overriden to throw exceptions (that will 
be wrapped in MalformedURLException) for a variety of reasons.


The ability for URLStreamHandler implementations to override the 
parseURL method seem to prevent this improvement unless we only do this 
for a subset of known, well-behaved URLStreamHandlers, which likely 
defeat the optimization by adding complexity.


Thanks!

/Claes

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Chris Hegarty]

On 18/01/16 14:09, Alan Bateman wrote:


On 17/01/2016 15:30, Claes Redestad wrote:

Hi,

please review this patch which might make URI.toURL more efficient

Webrev: http://cr.openjdk.java.net/~redestad/8147462
Bug: https://bugs.openjdk.java.net/browse/JDK-8147462

This patch exploits the fact that URI will apply the same validation
to the URI/URL specification for any valid non-opaque URL, thus it's
safe to use the component-based URL constructor. Also, URIs
representing malformed URLs will throw an exception as specified both
before and after. A number of simple test cases to capture and
document this was added to the existing java/net/URI/URItoURL jtreg test.

I think the change to URI looks okay but it would be good to include a
brief comment as otherwise it will be difficult for future maintainers
to understand.


+1.  For a long time we have been suggesting that anyone requiring
a URL should retrieve it from URI::toURL, so it is good to have a
fast(er) common path.

The handling of a null/empty host is a little esoteric, so a comment
would be good.

I do have a little discomfort about this change, since the toURL spec
specifically says it is "equivalent to evaluating the expression new 
URL(this.toString())". I wonder if your tests should cover this too,

i.e. the resulting URLs from toURL and URL(this.toString()) are equal?

-Chris.

Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Alan Bateman]

On 17/01/2016 15:30, Claes Redestad wrote:

Hi,

please review this patch which might make URI.toURL more efficient

Webrev: http://cr.openjdk.java.net/~redestad/8147462
Bug: https://bugs.openjdk.java.net/browse/JDK-8147462

This patch exploits the fact that URI will apply the same validation 
to the URI/URL specification for any valid non-opaque URL, thus it's 
safe to use the component-based URL constructor. Also, URIs 
representing malformed URLs will throw an exception as specified both 
before and after. A number of simple test cases to capture and 
document this was added to the existing java/net/URI/URItoURL jtreg test.
I think the change to URI looks okay but it would be good to include a 
brief comment as otherwise it will be difficult for future maintainers 
to understand.


In the test then it might be simpler to just check that the type of 
toURLException and newURLException are the same.


-Alan.

RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Claes Redestad]

Hi,

please review this patch which might make URI.toURL more efficient

Webrev: http://cr.openjdk.java.net/~redestad/8147462
Bug: https://bugs.openjdk.java.net/browse/JDK-8147462

This patch exploits the fact that URI will apply the same validation to 
the URI/URL specification for any valid non-opaque URL, thus it's safe 
to use the component-based URL constructor. Also, URIs representing 
malformed URLs will throw an exception as specified both before and 
after. A number of simple test cases to capture and document this was 
added to the existing java/net/URI/URItoURL jtreg test.


Microbenchmarks covering various URIs vary from neutral (for opaque) to 
1.5x (for the simplest URIs without query and fragment components), 
while also bringing in a small footprint improvement in jake builds.


While toURL() could arguably construct the URL file as 
path[?query][#fragment], the implementation I tried for this caused 
regressions in micros due to hitting C2 inlining limit.


Thanks!

/Claes