Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

Thanks!  Delivered without further changes.

That should be the end of this batch of classloader changes.

On Fri, Mar 2, 2018 at 7:00 AM, Roger Riggs  wrote:

> Hi Martin,
>
> I created the subtask for the release note[1], please review and update
> the summary and description.
> When it is done, close it as 'delivered'.
>
> Thanks, Roger
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8198956
>
>
>
> On 3/1/2018 10:32 PM, Martin Buchholz wrote:
>
>> On Wed, Feb 28, 2018 at 3:58 PM, Martin Buchholz 
>> wrote:
>>
>>
>>> 8198810: URLClassLoader does not specify behavior when URL array contains
>>> null
>>> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
>>> https://bugs.openjdk.java.net/browse/JDK-8198810
>>>
>>>   Pushed. Can someone at Oracle help with the release note?
>>
>
>

Re: RFR: Here are some URLClassPath patches

[Roger Riggs]

Hi Martin,

I created the subtask for the release note[1], please review and update 
the summary and description.

When it is done, close it as 'delivered'.

Thanks, Roger

[1] https://bugs.openjdk.java.net/browse/JDK-8198956


On 3/1/2018 10:32 PM, Martin Buchholz wrote:

On Wed, Feb 28, 2018 at 3:58 PM, Martin Buchholz 
wrote:



8198810: URLClassLoader does not specify behavior when URL array contains
null
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
https://bugs.openjdk.java.net/browse/JDK-8198810


  Pushed. Can someone at Oracle help with the release note?

Re: RFR: Here are some URLClassPath patches

[David Holmes]

On 2/03/2018 2:59 PM, Martin Buchholz wrote:

I have a patch to move those old jdi tests out of the ProblemList jail.
I thought we had an existing bug in hotspot-svc to do this, but now I 
can't find it.


I've created a sub-task of 8198803 for this and assigned it to you.

https://bugs.openjdk.java.net/browse/JDK-8198933


We can just use well-formed URLs.

http://cr.openjdk.java.net/~martin/webrevs/jdk/jdi-tests-malformed-urls/


Looks fine. The proof is in the testing of course :)

Thanks,
David

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

I have a patch to move those old jdi tests out of the ProblemList jail.
I thought we had an existing bug in hotspot-svc to do this, but now I can't
find it.

We can just use well-formed URLs.

http://cr.openjdk.java.net/~martin/webrevs/jdk/jdi-tests-malformed-urls/

Re: RFR: Here are some URLClassPath patches

[David Holmes]

On 2/03/2018 2:21 PM, Martin Buchholz wrote:

Ops.  8198810 is the id of the CSR, not the real bug - I should have
used 8198803.  So I may have broken a jira invariant.  Probably jcheck
should have caught my mistake.  What now?


Now you have to manually update the real bug with the changeset info and 
change its state etc. It will be fixed in build "master" initially then 
when the change hits a promoted build it will need to be changed to the 
actual build number.


And add a comment to the real bug about the incorrect number being used.

Thanks,
David


changeset:   49117:0a93645a57f1  qparent
user:martin
date:2018-03-01 19:01 -0800
8198810: URLClassLoader does not specify behavior when URL array contains
null
Reviewed-by: alanb, darcy, dholmes


On Thu, Mar 1, 2018 at 7:32 PM, Martin Buchholz  wrote:




On Wed, Feb 28, 2018 at 3:58 PM, Martin Buchholz 
wrote:




8198810: URLClassLoader does not specify behavior when URL array contains
null
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
https://bugs.openjdk.java.net/browse/JDK-8198810



  Pushed. Can someone at Oracle help with the release note?

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

Ops.  8198810 is the id of the CSR, not the real bug - I should have
used 8198803.  So I may have broken a jira invariant.  Probably jcheck
should have caught my mistake.  What now?

changeset:   49117:0a93645a57f1  qparent
user:martin
date:2018-03-01 19:01 -0800
8198810: URLClassLoader does not specify behavior when URL array contains
null
Reviewed-by: alanb, darcy, dholmes


On Thu, Mar 1, 2018 at 7:32 PM, Martin Buchholz  wrote:

>
>
> On Wed, Feb 28, 2018 at 3:58 PM, Martin Buchholz 
> wrote:
>
>>
>>
>> 8198810: URLClassLoader does not specify behavior when URL array contains
>> null
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
>> https://bugs.openjdk.java.net/browse/JDK-8198810
>>
>
>  Pushed. Can someone at Oracle help with the release note?
>
>

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Wed, Feb 28, 2018 at 3:58 PM, Martin Buchholz 
wrote:

>
>
> 8198810: URLClassLoader does not specify behavior when URL array contains
> null
> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
> https://bugs.openjdk.java.net/browse/JDK-8198810
>

 Pushed. Can someone at Oracle help with the release note?

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

OK, that was weird...
All I did for testing was follow

// This section should be uncommented if 8026517 is fixed.


but ...  8026517 is marked fixed!

So switching to ArrayDeque accidentally fixed 8026517 for real?!

8198810: URLClassLoader does not specify behavior when URL array contains
null
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
https://bugs.openjdk.java.net/browse/JDK-8198810

Re: RFR: Here are some URLClassPath patches

[Alan Bateman]

On 28/02/2018 18:04, Martin Buchholz wrote:

:

Too bad the URL[] argument is not consistently the last one, else one 
could  convert them all to varargs. Varargs-friendliness seems like an 
independent change.

Yes, completely separate issue and one that is already tracked:
https://bugs.openjdk.java.net/browse/JDK-8068425




I'm not really a java.net  guy (yet?) - I invite you 
to take over this sub-task of "improving URLClassLoader's constructors".

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Wed, Feb 28, 2018 at 6:17 AM, Alan Bateman 
wrote:

> On 28/02/2018 03:02, Martin Buchholz wrote:
>
> I should probably do more testing than usual when touching classloading...
>
> We have a jdi test that does this (hg blame finds duke as only author):
>
> public static ClassLoader classLoaderValue;
> {
> try {
> urls[0] = new URL("hi there");
> } catch (java.net.MalformedURLException ee) {
> }
> classLoaderValue = new URLClassLoader(urls);
> }
>
> :
>
> Can we get jdi team to fix their dodgy tests?
>
> I checked pre OpenJDK history but I'm none the wiser. I assume the
> original author of this test assumed that urls[0] would be set to
> placeholder URL, it's just the MalformedURLException (which is must catch
> because it's a checked exception) masked the test bug.
>

Yes, it's likely this is a classic example of "impossible exception
swallowing" that is no longer considered acceptable.  Of course converting
to an unchecked exception like AssertionError would have been correct and
would have caught the mistake.


> java.net.URLClassLoader and its implementation in URLClassPath
> implementation haven't had a lot of TLC which probably explains why the
> null issue didn't surface before. We also need to change the one-arg
> constructor to use varargs to avoid needing to create an explicit array for
> the common case that you want to create it with a single URL.
>

Too bad the URL[] argument is not consistently the last one, else one
could  convert them all to varargs.  Varargs-friendliness seems like an
independent change.

I'm not really a java.net guy (yet?) - I invite you to take over this
sub-task of "improving URLClassLoader's constructors".

Re: RFR: Here are some URLClassPath patches

[Alan Bateman]

On 28/02/2018 03:02, Martin Buchholz wrote:

I should probably do more testing than usual when touching classloading...

We have a jdi test that does this (hg blame finds duke as only author):

    public static ClassLoader classLoaderValue;
    {
        try {
            urls[0] = new URL("hi there");
        } catch (java.net.MalformedURLException ee) {
        }
        classLoaderValue = new URLClassLoader(urls);
    }

:

Can we get jdi team to fix their dodgy tests?
I checked pre OpenJDK history but I'm none the wiser. I assume the 
original author of this test assumed that urls[0] would be set to 
placeholder URL, it's just the MalformedURLException (which is must 
catch because it's a checked exception) masked the test bug.


java.net.URLClassLoader and its implementation in URLClassPath 
implementation haven't had a lot of TLC which probably explains why the 
null issue didn't surface before. We also need to change the one-arg 
constructor to use varargs to avoid needing to create an explicit array 
for the common case that you want to create it with a single URL.


-Alan

Re: RFR: Here are some URLClassPath patches

[mandy chung]

On 2/27/18 9:03 PM, Martin Buchholz wrote:

8198808: jdi tests failing after JDK-8198484
http://cr.openjdk.java.net/~martin/webrevs/jdk/jdi-ProblemList/ 


https://bugs.openjdk.java.net/browse/JDK-8198808


+1

Mandy

Re: RFR: Here are some URLClassPath patches

[David Holmes]

Looks good.

Thanks,
David

On 28/02/2018 3:03 PM, Martin Buchholz wrote:

8198808: jdi tests failing after JDK-8198484
http://cr.openjdk.java.net/~martin/webrevs/jdk/jdi-ProblemList/
https://bugs.openjdk.java.net/browse/JDK-8198808

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

8198808: jdi tests failing after JDK-8198484
http://cr.openjdk.java.net/~martin/webrevs/jdk/jdi-ProblemList/
https://bugs.openjdk.java.net/browse/JDK-8198808

Re: RFR: Here are some URLClassPath patches

[Joseph D. Darcy]

Hi Martin,

Until we figure out the best course of action, please problem list the 
failing test.


Thanks,

-Joe

On 2/27/2018 7:02 PM, Martin Buchholz wrote:

I should probably do more testing than usual when touching classloading...

We have a jdi test that does this (hg blame finds duke as only author):

 public static ClassLoader classLoaderValue;
 {
 try {
 urls[0] = new URL("hi there");
 } catch (java.net.MalformedURLException ee) {
 }
 classLoaderValue = new URLClassLoader(urls);
 }

The assignment to urls[0] never succeeds (!), so URLClassLoader(URL[]) is
called with an array holding a null.  Which should cause a NPE, but did not
prior to JDK-8198484 .
ArrayDeque does not permit null elements!

Should we accept the NPE as a bug fix and file a retroactive CSR for that,
or should we continue to compatibly tolerate null URLs (yuk)?

Can we get jdi team to fix their dodgy tests?

Re: RFR: Here are some URLClassPath patches

[David Holmes]

Hi Martin,

On 28/02/2018 1:02 PM, Martin Buchholz wrote:

I should probably do more testing than usual when touching classloading...

We have a jdi test that does this (hg blame finds duke as only author):

     public static ClassLoader classLoaderValue;
     {
         try {
             urls[0] = new URL("hi there");
         } catch (java.net.MalformedURLException ee) {
         }
         classLoaderValue = new URLClassLoader(urls);
     }

The assignment to urls[0] never succeeds (!), so URLClassLoader(URL[]) 
is called with an array holding a null.  Which should cause a NPE, but 
did not prior to JDK-8198484 
.

ArrayDeque does not permit null elements!

Should we accept the NPE as a bug fix and file a retroactive CSR for 
that, or should we continue to compatibly tolerate null URLs (yuk)?


As I wrote in the bug report the tolerance for nulls is very limited. 
Seems you can have them in the initial URL[] but if you actually try to 
use one you get NPE. So accepting a null entry in the URL[] seems rather 
pointless - and addURL explicitly states that null is ignored. So I'd 
argue for the CSR so we can at least consider whether we need tolerate this.



Can we get jdi team to fix their dodgy tests?


Yes. Depending on which way we go we may need a new bug, otherwise this 
one can be assigned back to hotspot->svc. I'm also perplexed by the test 
logic.


Thanks,
David

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

I should probably do more testing than usual when touching classloading...

We have a jdi test that does this (hg blame finds duke as only author):

public static ClassLoader classLoaderValue;
{
try {
urls[0] = new URL("hi there");
} catch (java.net.MalformedURLException ee) {
}
classLoaderValue = new URLClassLoader(urls);
}

The assignment to urls[0] never succeeds (!), so URLClassLoader(URL[]) is
called with an array holding a null.  Which should cause a NPE, but did not
prior to JDK-8198484 .
ArrayDeque does not permit null elements!

Should we accept the NPE as a bug fix and file a retroactive CSR for that,
or should we continue to compatibly tolerate null URLs (yuk)?

Can we get jdi team to fix their dodgy tests?

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Mon, Feb 26, 2018 at 3:38 PM, mandy chung  wrote:

>
>
> On 2/26/18 3:26 PM, Martin Buchholz wrote:
>
>
> Looks okay.  I also think no need to have a separate copyToArrayDeque
>> method and just inline in the constructor.
>>
>
> It's used twice.  Also, it's likely to be replaced someday when we decide
> what to do with lambdas, so good to keep as a separate method.
>
>
> The first use can be replaced with very simple code:
>
> ArrayList path = new ArrayList<>(urls.length);
> ArrayDeque unopenedUrls = new ArrayDeque<>(urls.length);
> for (URL url : urls) {
> path.add(url);
> unopenedUrls.add(url);
> }
>
> I had trouble deciding which way was better, so let's do it your way!
toArrayDeque is gone.

Re: RFR: Here are some URLClassPath patches

[mandy chung]

On 2/26/18 3:26 PM, Martin Buchholz wrote:



Looks okay.  I also think no need to have a separate
copyToArrayDeque method and just inline in the constructor.


It's used twice.  Also, it's likely to be replaced someday when we 
decide what to do with lambdas, so good to keep as a separate method.


The first use can be replaced with very simple code:

ArrayList path = new ArrayList<>(urls.length);
ArrayDeque unopenedUrls = new ArrayDeque<>(urls.length);
for (URL url : urls) {
path.add(url);
unopenedUrls.add(url);
}

Mandy

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Mon, Feb 26, 2018 at 3:06 PM, mandy chung  wrote:

>
>
> On 2/21/18 12:30 PM, Martin Buchholz wrote:
> line 63-64: ident further to the right as line 62 is the condition.
>
>
Whitespace rejiggered.


> Looks okay.  I also think no need to have a separate copyToArrayDeque
> method and just inline in the constructor.
>

It's used twice.  Also, it's likely to be replaced someday when we decide
what to do with lambdas, so good to keep as a separate method.

Re: RFR: Here are some URLClassPath patches

[mandy chung]

On 2/26/18 3:10 PM, Martin Buchholz wrote:



On Mon, Feb 26, 2018 at 3:06 PM, mandy chung > wrote:




8198480: Improve ClassLoaders static init block
http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/

https://bugs.openjdk.java.net/browse/JDK-8198480




Can you rename initialModuleName to mainModule as Alan suggests
(also in the comment)?


It's the other way around - I renamed it from mainModuleName to 
initialModuleName based on Alan's comment.  (and the code elsewhere 
continues to be inconsistent on this)


OK.   I agree we should follow up and make them consistent.

Mandy

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Mon, Feb 26, 2018 at 3:06 PM, mandy chung  wrote:

>
> 8198480: Improve ClassLoaders static init block
> http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/
> https://bugs.openjdk.java.net/browse/JDK-8198480
>
>
> Can you rename initialModuleName to mainModule as Alan suggests (also in
> the comment)?
>

It's the other way around - I renamed it from mainModuleName to
initialModuleName based on Alan's comment.  (and the code elsewhere
continues to be inconsistent on this)

Re: RFR: Here are some URLClassPath patches

[mandy chung]

On 2/21/18 12:30 PM, Martin Buchholz wrote:

OK, we have a reworked set of patches.

(In my corner of openjdk we generally use real javadoc on private 
elements.)


I reverted private doc comment style to the current maddening 
inconsistency, except I couldn't restrain myself from fixing


-    // ACC used when loading classes and resources */
+    // ACC used when loading classes and resources

I changed ArrayDeque.push to addFirst, as Peter suggested.


8198480: Improve ClassLoaders static init block
http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/ 


https://bugs.openjdk.java.net/browse/JDK-8198480



Can you rename initialModuleName to mainModule as Alan suggests (also in 
the comment)?

line 63-64: ident further to the right as line 62 is the condition.

8198481: Coding style cleanups for 
src/java.base/share/classes/jdk/internal/loader
http://cr.openjdk.java.net/~martin/webrevs/jdk/loader-style/ 


https://bugs.openjdk.java.net/browse/JDK-8198481


Looks okay.



8198482: The URLClassPath field "urls" should be renamed to "unopenedUrls"
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-rename-urls/ 


https://bugs.openjdk.java.net/browse/JDK-8198482



+1


8198484: URLClassPath should use an ArrayDeque instead of a Stack
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/ 


https://bugs.openjdk.java.net/browse/JDK-8198484



Looks okay.  I also think no need to have a separate copyToArrayDeque 
method and just inline in the constructor.



8198485: Simplify a URLClassPath constructor
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-simplify-constructor/ 


https://bugs.openjdk.java.net/browse/JDK-8198485



+1

Mandy

Re: RFR: Here are some URLClassPath patches

[Paul Sandoz]

> On Feb 26, 2018, at 1:25 PM, Martin Buchholz  wrote:
> 
> 
> 
> On Mon, Feb 26, 2018 at 9:28 AM, Paul Sandoz  > wrote:
> 
> 
> > On Feb 23, 2018, at 2:09 PM, Martin Buchholz  > > wrote:
> >
> > [+Paul]
> >
> > On Fri, Feb 23, 2018 at 6:28 AM, Alan Bateman  > > wrote:
> >>
> >> 8198484: URLClassPath should use an ArrayDeque instead of a Stack
> >> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/ 
> >> 
> >> https://bugs.openjdk.java.net/browse/JDK-8198484 
> >> 
> > Can copyToArrayDeque use addAll?
> >
> 
> 
> > On Feb 23, 2018, at 2:09 PM, Martin Buchholz  > > wrote:
> 
> > Not directly, because addAll uses a lambda, and it's too early in the 
> > bootstrap for lambdas.
> >
> > We could delambdafy ArrayDeque, plausibly because ArrayDeque is a 
> > super-core class, perhaps reengineering ArrayDeque(Collection) and/or 
> > addAll(Collection).
> >
> 
> Some data on how many lambdas/methods refs are used by the core collection 
> classes could help. I would be wary of going on a lambda purge right now and 
> biasing certain collection classes towards their use at startup if we can 
> avoid that with careful management.
> 
> I would be tempted to drop the method copyToArrayDeque and just rely on the 
> push method (even though it uses synchronized), then add a comment to the 
> unopenedUrls field and/or in the constructor.
> 
> 
> I prefer to keep things as I have them for now.  Calling push requires an 
> extra copy to create the array,

Ok, fair point,
Paul.

> and even though that overhead is swamped by other inefficiencies, it's a tax 
> on every single java program (and the tax is higher at Google, where we have 
> trouble fitting the classpath into Linux' 128 kb per-arg command line length 
> limit).
> 

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Mon, Feb 26, 2018 at 9:28 AM, Paul Sandoz  wrote:

>
>
> > On Feb 23, 2018, at 2:09 PM, Martin Buchholz 
> wrote:
> >
> > [+Paul]
> >
> > On Fri, Feb 23, 2018 at 6:28 AM, Alan Bateman 
> wrote:
> >>
> >> 8198484: URLClassPath should use an ArrayDeque instead of a Stack
> >> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/
> >> https://bugs.openjdk.java.net/browse/JDK-8198484
> > Can copyToArrayDeque use addAll?
> >
>
>
> > On Feb 23, 2018, at 2:09 PM, Martin Buchholz 
> wrote:
>
> > Not directly, because addAll uses a lambda, and it's too early in the
> bootstrap for lambdas.
> >
> > We could delambdafy ArrayDeque, plausibly because ArrayDeque is a
> super-core class, perhaps reengineering ArrayDeque(Collection) and/or
> addAll(Collection).
> >
>
> Some data on how many lambdas/methods refs are used by the core collection
> classes could help. I would be wary of going on a lambda purge right now
> and biasing certain collection classes towards their use at startup if we
> can avoid that with careful management.
>
> I would be tempted to drop the method copyToArrayDeque and just rely on
> the push method (even though it uses synchronized), then add a comment to
> the unopenedUrls field and/or in the constructor.
>
>
I prefer to keep things as I have them for now.  Calling push requires an
extra copy to create the array, and even though that overhead is swamped by
other inefficiencies, it's a tax on every single java program (and the tax
is higher at Google, where we have trouble fitting the classpath into
Linux' 128 kb per-arg command line length limit).

Re: RFR: Here are some URLClassPath patches

[Paul Sandoz]

> On Feb 23, 2018, at 2:09 PM, Martin Buchholz  wrote:
> 
> [+Paul]
> 
> On Fri, Feb 23, 2018 at 6:28 AM, Alan Bateman  wrote:
>> 
>> 8198484: URLClassPath should use an ArrayDeque instead of a Stack
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/
>> https://bugs.openjdk.java.net/browse/JDK-8198484
> Can copyToArrayDeque use addAll?
> 


> On Feb 23, 2018, at 2:09 PM, Martin Buchholz  wrote:

> Not directly, because addAll uses a lambda, and it's too early in the 
> bootstrap for lambdas.
> 
> We could delambdafy ArrayDeque, plausibly because ArrayDeque is a super-core 
> class, perhaps reengineering ArrayDeque(Collection) and/or addAll(Collection).
> 

Some data on how many lambdas/methods refs are used by the core collection 
classes could help. I would be wary of going on a lambda purge right now and 
biasing certain collection classes towards their use at startup if we can avoid 
that with careful management.

I would be tempted to drop the method copyToArrayDeque and just rely on the 
push method (even though it uses synchronized), then add a comment to the 
unopenedUrls field and/or in the constructor.


> Or perhaps lambda team has a plan to allow lambdas to be used in corest of 
> core java sometime soon?

No current plans to more formally attack this gnarly problem. A start would be 
to gather data on the VM boot code-paths to better understand the sensitive 
areas, from that we might consider black listing some classes so a compiler 
failure is produced. IIRC the circularity errors can often confusing, it might 
be possible to place a check for the required VM init level, thereby producing 
a clearer runtime error. Lazier initialization might help too (i hope with 
constant dynamic stuff, where even though it’s using j.l.invoke it will move 
stuff off the critical path, while also spreading the cost of startup).

Paul.

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

[+Paul]

On Fri, Feb 23, 2018 at 6:28 AM, Alan Bateman 
wrote:

>
> 8198484: URLClassPath should use an ArrayDeque instead of a Stack
> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/
> https://bugs.openjdk.java.net/browse/JDK-8198484
>
> Can copyToArrayDeque use addAll?
>

Not directly, because addAll uses a lambda, and it's too early in the
bootstrap for lambdas.

We could delambdafy ArrayDeque, plausibly because ArrayDeque is a
super-core class, perhaps reengineering ArrayDeque(Collection) and/or
addAll(Collection).

Or perhaps lambda team has a plan to allow lambdas to be used in corest of
core java sometime soon?

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Fri, Feb 23, 2018 at 6:28 AM, Alan Bateman 
wrote:

> Just getting to the updated webrev now.
>
> On 21/02/2018 20:30, Martin Buchholz wrote:
>
> :
>
>
> 8198480: Improve ClassLoaders static init block
> http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/
> https://bugs.openjdk.java.net/browse/JDK-8198480
>
> The value of jdk.module.main is the name of the "initial module" so better
> to use that instead term of "main module".
>
>
Done, but I see elsewhere the same convention, that jigsaw team might want
to make consistent:

---
String mainModule;

String mainModule = System.getProperty("jdk.module.main");

 * {@code jdk.module.main}
 * The module name of the initial/main module
 * {@code jdk.module.main.class}
 * The main class name of the initial module

SetMainModule(const char *s)
---

Re: RFR: Here are some URLClassPath patches

[Alan Bateman]

Just getting to the updated webrev now.

On 21/02/2018 20:30, Martin Buchholz wrote:

:


8198480: Improve ClassLoaders static init block
http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/ 


https://bugs.openjdk.java.net/browse/JDK-8198480
The value of jdk.module.main is the name of the "initial module" so 
better to use that instead term of "main module".





8198481: Coding style cleanups for 
src/java.base/share/classes/jdk/internal/loader
http://cr.openjdk.java.net/~martin/webrevs/jdk/loader-style/ 


https://bugs.openjdk.java.net/browse/JDK-8198481

Looks okay.



8198482: The URLClassPath field "urls" should be renamed to "unopenedUrls"
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-rename-urls/ 


https://bugs.openjdk.java.net/browse/JDK-8198482

Looks okay.



8198484: URLClassPath should use an ArrayDeque instead of a Stack
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/ 


https://bugs.openjdk.java.net/browse/JDK-8198484

Can copyToArrayDeque use addAll?




8198485: Simplify a URLClassPath constructor
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-simplify-constructor/ 


https://bugs.openjdk.java.net/browse/JDK-8198485


Looks okay.

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Wed, Feb 21, 2018 at 7:48 AM, Roger Riggs  wrote:

>
> On the style changes in URLClassPath-ArrayDeque, about declarations like:
>
> ArrayList path = new ArrayList<>();
>
> It had been a recommended style to use the abstract type (List) on the
> left hand side
> and only use the concrete type where necessary.
>

There's a debate about whether using concrete types in the implementation
is clearer.  I happen to think so.   But in performance critical classes
the concrete class is more important to the maintainer (I'm thinking
"ArrayDeque" not "Deque" when I maintain this code), and is also likely to
help the JVM.

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

OK, we have a reworked set of patches.

(In my corner of openjdk we generally use real javadoc on private elements.)

I reverted private doc comment style to the current maddening
inconsistency, except I couldn't restrain myself from fixing

-// ACC used when loading classes and resources */
+// ACC used when loading classes and resources

I changed ArrayDeque.push to addFirst, as Peter suggested.


8198480: Improve ClassLoaders static init block
http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/
https://bugs.openjdk.java.net/browse/JDK-8198480

8198481: Coding style cleanups for
src/java.base/share/classes/jdk/internal/loader
http://cr.openjdk.java.net/~martin/webrevs/jdk/loader-style/
https://bugs.openjdk.java.net/browse/JDK-8198481

8198482: The URLClassPath field "urls" should be renamed to "unopenedUrls"
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-rename-urls/
https://bugs.openjdk.java.net/browse/JDK-8198482

8198484: URLClassPath should use an ArrayDeque instead of a Stack
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/
https://bugs.openjdk.java.net/browse/JDK-8198484

8198485: Simplify a URLClassPath constructor
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-simplify-constructor/
https://bugs.openjdk.java.net/browse/JDK-8198485

Re: RFR: Here are some URLClassPath patches

[mark . reinhold]

2018/2/21 2:28:19 -0500, alan.bate...@oracle.com:
> On 21/02/2018 06:08, Martin Buchholz wrote:
>> :
>> 
>> 8198481: Coding style cleanups for 
>> src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/loader-style/ 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8198481
> 
> This changes comments on private fields from // to /**. Are you 
> generating javadoc on private members of internal classes or why are you 
> changing all these fields?

The use of // rather than /** on private members is intentional,
to make it clear to the reader that these are internal elements
rather than API.

You might not like it, but please don't change it.

- Mark

Re: RFR: Here are some URLClassPath patches

[Roger Riggs]

Hi Martin,

Generally, I would leave code alone that does not have a good reason for 
changing.

That goes for commenting conventions (// vs /*) too.

On the style changes in URLClassPath-ArrayDeque, about declarations like:

ArrayList path = new ArrayList<>();

It had been a recommended style to use the abstract type (List) on the 
left hand side

and only use the concrete type where necessary.

But with the addition of 'var' local type inference, perhaps that would 
an option here.


(Not trying to start a style war).

$.02, Roger


On 2/21/2018 1:08 AM, Martin Buchholz wrote:

At Google I spend a lot of time staring unproductively at classloader code,
and it's always hard for me to resist the urge to clean it up.  So here are
some patches that should be relatively uncontroversial, and may prepare for
more radical changes later.


8198484: URLClassPath should use an ArrayDeque instead of a Stack
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/
https://bugs.openjdk.java.net/browse/JDK-8198484

Re: RFR: Here are some URLClassPath patches

[Alan Bateman]

On 21/02/2018 06:08, Martin Buchholz wrote:

:

8198485: Simplify a URLClassPath constructor
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-simplify-constructor/ 


https://bugs.openjdk.java.net/browse/JDK-8198485

This one looks okay to me.

-Alan

Re: RFR: Here are some URLClassPath patches

[Peter Levart]

Hi Martin,

I checked this one...

On 02/21/2018 07:08 AM, Martin Buchholz wrote:

8198484: URLClassPath should use an ArrayDeque instead of a Stack
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/
https://bugs.openjdk.java.net/browse/JDK-8198484


I admit I had to study the Stack API first as I don't regularly use it 
nowadays ;-). You seem to be using the following replacements consistently:


Stack vs. ArrayDequeue

push(e) vs. addFirst(e)
add(0, e) vs. addLast(e)
isEmpty() / pop() vs. pollFirst()

...but then in push(URL[]), you use:

push(e) vs. push(e)

Deque.addFirst(e) is equivalent to Deque.push(e), but it would be nice 
to keep using the same method consistently in one class.



Regards, Peter

Re: RFR: Here are some URLClassPath patches

[Alan Bateman]

On 21/02/2018 06:08, Martin Buchholz wrote:

:

8198480: Improve ClassLoaders static init block
http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/ 


https://bugs.openjdk.java.net/browse/JDK-8198480

This initializer has changed a few times and I agree it's a bit awkward 
to understand when there is a class path or not. So I think you're 
rejigging is okay, as is the rewording of the comment. The blank line at 
L45 was intentional, no need to remove that.


-Alan

Re: RFR: Here are some URLClassPath patches

[Alan Bateman]

On 21/02/2018 06:08, Martin Buchholz wrote:

:

8198482: The URLClassPath field "urls" should be renamed to "unopenedUrls"
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-rename-urls/ 


https://bugs.openjdk.java.net/browse/JDK-8198482


I think this looks okay.

-Alan

Re: RFR: Here are some URLClassPath patches

[Alan Bateman]

On 21/02/2018 06:08, Martin Buchholz wrote:

:

8198481: Coding style cleanups for 
src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
http://cr.openjdk.java.net/~martin/webrevs/jdk/loader-style/ 


https://bugs.openjdk.java.net/browse/JDK-8198481

This changes comments on private fields from // to /**.  Are you 
generating javadoc on private members of internal classes or why are you 
changing all these fields?


-Alan