hg: jigsaw/jake/langtools: 2 new changesets

[jonathan . gibbons]

Changeset: 562e51ce8df5
Author:jjg
Date:  2016-04-25 15:09 -0700
URL:   http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/562e51ce8df5

minor adjustment to javadoc

! src/java.compiler/share/classes/javax/lang/model/util/Elements.java

Changeset: 116f2acc53e6
Author:jjg
Date:  2016-04-25 15:41 -0700
URL:   http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/116f2acc53e6

remove javac support for old -Xpatch

! src/jdk.compiler/share/classes/com/sun/tools/javac/file/Locations.java

Re: automatic modules leaking types when using addmods

[Peter Levart]

On 04/25/2016 09:09 PM, Alex Buckley wrote:
Consider an automatic module MA which is required by an explicit named 
module MN. MA could mention, in its exported API, public types from 
other automatic modules (e.g. seems reasonable for jackson.databind to 
have a method whose return type is from jackson.core) so we want MN to 
read those other automatic modules.


The subtlety is that "other automatic modules" doesn't mean "every 
legacy JAR on the modulepath". It means "every legacy JAR on the 
modulepath which is required by an explicit named module, or is 
otherwise given by -addmods".


Alex


In other words, automatic module implicitly reads all modules much like 
it would have "requires public" specified for all of them in its 
hypothetical module-info (or just for automatic modules?)...


Regards, Peter



On 4/25/2016 12:22 AM, Paul Bakker wrote:

I now understand (and tested) that when I use any automatic module
from a named module, the named module gets implicit readability to
*all* automatic modules. What is the reasoning behind this?

If a module A has a dependency on B and C, I get the impression
during migration that "requires B" would be sufficient for module A.
Up until the point that B is migrated to a named module, because than
suddenly A needs a "requires C" as well. Of course automatic modules
will never be an exact representation of a fully migrated situation,
but it would be nice to get as close as possible.

Paul



On 25 Apr 2016, at 08:59, Alan Bateman 
wrote:



On 25/04/2016 07:35, Paul Bakker wrote:

That doesn't seem to be the case, I can run successfully, as long
as I have the right -addmods. I've pushed my example here if you
want to take a further look at it:
https://github.com/paulbakker/automaticmodules-example



I wasn't clear. My comment was on when the scenario is changed
slightly to have an additional explicit module in the picture, say
java.desktop. In that case then javac is granting implicit
readability and so requiring jackson.databind allows the module to
make use of types in java.desktop. It may be specific to system
modules but I will create a bug on that.

-Alan.

Re: automatic modules leaking types when using addmods

[Alex Buckley]

Consider an automatic module MA which is required by an explicit named 
module MN. MA could mention, in its exported API, public types from 
other automatic modules (e.g. seems reasonable for jackson.databind to 
have a method whose return type is from jackson.core) so we want MN to 
read those other automatic modules.


The subtlety is that "other automatic modules" doesn't mean "every 
legacy JAR on the modulepath". It means "every legacy JAR on the 
modulepath which is required by an explicit named module, or is 
otherwise given by -addmods".


Alex

On 4/25/2016 12:22 AM, Paul Bakker wrote:

I now understand (and tested) that when I use any automatic module
from a named module, the named module gets implicit readability to
*all* automatic modules. What is the reasoning behind this?

If a module A has a dependency on B and C, I get the impression
during migration that "requires B" would be sufficient for module A.
Up until the point that B is migrated to a named module, because than
suddenly A needs a "requires C" as well. Of course automatic modules
will never be an exact representation of a fully migrated situation,
but it would be nice to get as close as possible.

Paul



On 25 Apr 2016, at 08:59, Alan Bateman 
wrote:



On 25/04/2016 07:35, Paul Bakker wrote:

That doesn't seem to be the case, I can run successfully, as long
as I have the right -addmods. I've pushed my example here if you
want to take a further look at it:
https://github.com/paulbakker/automaticmodules-example



I wasn't clear. My comment was on when the scenario is changed
slightly to have an additional explicit module in the picture, say
java.desktop. In that case then javac is granting implicit
readability and so requiring jackson.databind allows the module to
make use of types in java.desktop. It may be specific to system
modules but I will create a bug on that.

-Alan.

Re: RFR: JDK-8132994 - /modules and /packages should not be parsed by the jimage parser

[Sundararajan Athijegannathan]

"if(" in second if statement [missing whitespace after keyword]

+1

-Sundar

On 4/25/2016 8:08 PM, Jim Laskey (Oracle) wrote:
> Changed to special case parse /modules/ and /packages/.  Will actually reduce 
> string table size slightly (better common string match.)
>
> http://cr.openjdk.java.net/~jlaskey/8132994/webrev/index.html 
> 
> https://bugs.openjdk.java.net/browse/JDK-8132994 
> 
>

RFR: JDK-8132994 - /modules and /packages should not be parsed by the jimage parser

[Jim Laskey (Oracle)]

Changed to special case parse /modules/ and /packages/.  Will actually reduce 
string table size slightly (better common string match.)

http://cr.openjdk.java.net/~jlaskey/8132994/webrev/index.html 

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

Re: RFR JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

[Claes Redestad]

On 2016-04-25 15:02, Alan Bateman wrote:

On 25/04/2016 13:54, Claes Redestad wrote:

Hi,

I think this looks good, but since these WeakPairMaps won't be used 
for many applications I wonder if it's worth keeping the 
implementation lazy, for example by moving the maps to a holder class:


http://cr.openjdk.java.net/~redestad/scratch/transients.01/

I verified this avoids loading the WeakPairMap class for various 
small programs and should be neutral in other regards.
This looks like it saves loading one class and not sure that it's 
worth it. Also I expect there will be quite a bit of refactoring and 
changes as we work through issues over the next months few months.


Sure - also saves creating and holding the three maps eagerly - but I 
agree the saving might not justify the added complexity in this case.


/Claes

Re: RFR JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

[Alan Bateman]

On 25/04/2016 13:54, Claes Redestad wrote:

Hi,

I think this looks good, but since these WeakPairMaps won't be used 
for many applications I wonder if it's worth keeping the 
implementation lazy, for example by moving the maps to a holder class:


http://cr.openjdk.java.net/~redestad/scratch/transients.01/

I verified this avoids loading the WeakPairMap class for various small 
programs and should be neutral in other regards.
This looks like it saves loading one class and not sure that it's worth 
it. Also I expect there will be quite a bit of refactoring and changes 
as we work through issues over the next months few months.


-Alan

Re: RFR JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

[Alan Bateman]

On 25/04/2016 12:13, Peter Levart wrote:

Hi Alan,

I created an issue for this:

JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

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


I did what you suggested, renamed type parameters to , 
split long line in Module and modified the test so that it now waits 
for entry to be expunged for up to 5 seconds when it is expected to be 
expunged but returns as soon as it detects that the entry is gone 
(usually in 1st 100 ms). It still waits just 500 ms when the entry is 
expected to remain in the map. False negatives should be eliminated 
this way and the test still doesn't waist plenty of execution time.


Here's latest webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.04/ 


I think using  makes it a lot more readable - thanks.

On gcAndWaitRemoved then I wonder if it would be more robust to have a 
variant that polls indefinitely. 5s is a long time but with fastdebug 
builds, maybe a looping thread or processes left over from a previous 
test runs, virtual machines, then it might be long enough.





If this is accepted then I would need to know via which repo this 
should be pushed (jdk9/dev or jake).
It should be okay to push this to jdk9/dev and we'll pull the changes 
into jake. In general then we will be iterating on the module code for 
some time and so will be using the jake sandbox for that.


-Alan

Re: RFR JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

[Claes Redestad]

Hi,

I think this looks good, but since these WeakPairMaps won't be used for 
many applications I wonder if it's worth keeping the implementation 
lazy, for example by moving the maps to a holder class:


http://cr.openjdk.java.net/~redestad/scratch/transients.01/

I verified this avoids loading the WeakPairMap class for various small 
programs and should be neutral in other regards.


Thanks!

/Claes

On 2016-04-25 13:13, Peter Levart wrote:

Hi Alan,

I created an issue for this:

JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

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


I did what you suggested, renamed type parameters to , 
split long line in Module and modified the test so that it now waits 
for entry to be expunged for up to 5 seconds when it is expected to be 
expunged but returns as soon as it detects that the entry is gone 
(usually in 1st 100 ms). It still waits just 500 ms when the entry is 
expected to remain in the map. False negatives should be eliminated 
this way and the test still doesn't waist plenty of execution time.


Here's latest webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.04/ 




If this is accepted then I would need to know via which repo this 
should be pushed (jdk9/dev or jake).


Regards, Peter


On 04/24/2016 10:16 AM, Alan Bateman wrote:


On 22/04/2016 14:42, Peter Levart wrote:

:

I tried to reduce the complexity of WeakPairMap as much as I could. 
I added some docs that describe the architecture. Hopefully this is 
now easier to grasp:


http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.03/ 


I think this looks quite good.

As there are two keys then what would you think about renaming the 
type parameters to ?


In the test when I wonder if 500ms is enough to guarantee that 
references has been queued, esp. on loaded machines, maybe a previous 
test has several something loop for example.


In terms of the startup then only WeakPairMap should be loaded so I 
think that is okay. We'll find that -XaddExports will be a bit more 
expensive as will trigger quite a bit of initialization and class 
loading but I think that is okay too.


A minor comment on Module L518 is that we might want to split this 
line to avoid it being too long. Alternatively, just replace 
exportedToAll, exportedToOther and exportedToAllUnnamed with "exports".




Another possibility for transientReads and transientExports (but not 
for transientUses) could be if each instance of Module object held a 
unique long id allocated at its creation from say AtomicLong. You 
could then construct maps with Long ids instead of Module(s), but 
that has a drawback that some Module (say a system module or a 
module of an app server) could accumulate ids from modules long gone 
(for example when some app in an app server is redeployed multiple 
times) so this would be a memory leak...

I agree, particularly with exports (not reads, at least not anymore).



a single global WeakPairMap has an advantage over individual 
WeakSet(s) per Module instance because it is accessed more 
frequently so expunging of stale entries happens more promptly.

True. Another advantage is that it drops 3 instance fields.

-Alan.

RFR JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

[Peter Levart]

Hi Alan,

I created an issue for this:

JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

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


I did what you suggested, renamed type parameters to , split 
long line in Module and modified the test so that it now waits for entry 
to be expunged for up to 5 seconds when it is expected to be expunged 
but returns as soon as it detects that the entry is gone (usually in 1st 
100 ms). It still waits just 500 ms when the entry is expected to remain 
in the map. False negatives should be eliminated this way and the test 
still doesn't waist plenty of execution time.


Here's latest webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.04/


If this is accepted then I would need to know via which repo this should 
be pushed (jdk9/dev or jake).


Regards, Peter


On 04/24/2016 10:16 AM, Alan Bateman wrote:


On 22/04/2016 14:42, Peter Levart wrote:

:

I tried to reduce the complexity of WeakPairMap as much as I could. I 
added some docs that describe the architecture. Hopefully this is now 
easier to grasp:


http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.03/ 


I think this looks quite good.

As there are two keys then what would you think about renaming the 
type parameters to ?


In the test when I wonder if 500ms is enough to guarantee that 
references has been queued, esp. on loaded machines, maybe a previous 
test has several something loop for example.


In terms of the startup then only WeakPairMap should be loaded so I 
think that is okay. We'll find that -XaddExports will be a bit more 
expensive as will trigger quite a bit of initialization and class 
loading but I think that is okay too.


A minor comment on Module L518 is that we might want to split this 
line to avoid it being too long. Alternatively, just replace 
exportedToAll, exportedToOther and exportedToAllUnnamed with "exports".




Another possibility for transientReads and transientExports (but not 
for transientUses) could be if each instance of Module object held a 
unique long id allocated at its creation from say AtomicLong. You 
could then construct maps with Long ids instead of Module(s), but 
that has a drawback that some Module (say a system module or a module 
of an app server) could accumulate ids from modules long gone (for 
example when some app in an app server is redeployed multiple times) 
so this would be a memory leak...

I agree, particularly with exports (not reads, at least not anymore).



a single global WeakPairMap has an advantage over individual 
WeakSet(s) per Module instance because it is accessed more frequently 
so expunging of stale entries happens more promptly.

True. Another advantage is that it drops 3 instance fields.

-Alan.

Re: automatic modules leaking types when using addmods

[Paul Bakker]

I now understand (and tested) that when I use any automatic module from a named 
module, the named module gets implicit readability to *all* automatic modules. 
What is the reasoning behind this? 

If a module A has a dependency on B and C, I get the impression during 
migration that "requires B" would be sufficient for module A. Up until the 
point that B is migrated to a named module, because than suddenly A needs a 
"requires C" as well. Of course automatic modules will never be an exact 
representation of a fully migrated situation, but it would be nice to get as 
close as possible.

Paul


> On 25 Apr 2016, at 08:59, Alan Bateman  wrote:
> 
> 
> 
> On 25/04/2016 07:35, Paul Bakker wrote:
>> That doesn't seem to be the case, I can run successfully, as long as I have 
>> the right -addmods.
>> I've pushed my example here if you want to take a further look at it: 
>> https://github.com/paulbakker/automaticmodules-example 
>> 
>> 
> I wasn't clear. My comment was on when the scenario is changed slightly to 
> have an additional explicit module in the picture, say java.desktop. In that 
> case then javac is granting implicit readability and so requiring 
> jackson.databind allows the module to make use of types in java.desktop. It 
> may be specific to system modules but I will create a bug on that.
> 
> -Alan.

Re: automatic modules leaking types when using addmods

[Alan Bateman]

On 25/04/2016 07:35, Paul Bakker wrote:
That doesn't seem to be the case, I can run successfully, as long as I 
have the right -addmods.
I've pushed my example here if you want to take a further look at it: 
https://github.com/paulbakker/automaticmodules-example


I wasn't clear. My comment was on when the scenario is changed slightly 
to have an additional explicit module in the picture, say java.desktop. 
In that case then javac is granting implicit readability and so 
requiring jackson.databind allows the module to make use of types in 
java.desktop. It may be specific to system modules but I will create a 
bug on that.


-Alan.