Re: RFR 6425769: jmx remote bind address

[Jaroslav Bachorik]

Adding core libs.

On 2.11.2015 11:38, Severin Gehwolf wrote:

Hi,

Here is a patch addressing JDK-6425769. The issue is that the JMX agent
binds to the wildcard address by default, preventing users to use
system properties for JMX agents on multi-homed hosts. Given a host
with local network interfaces, 192.168.0.1 and 192.168.0.2 say, it's
impossible to start two JMX agents binding to fixed ports but to
different network interfaces, 192.168.0.1:{9111,9112} and
192.168.0.2:{9111,9112} say.

The JDK would bind to all local interfaces by default. In the above
example to 192.168.0.1 *and* 192.168.0.2. The effect is that the second
Java process would get a "Connection refused" error because another
process has already been bound to the specified JMX/RMI port pairs.

Bug: https://bugs.openjdk.java.net/browse/JDK-6425769
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/00/

Testing done:
jdk_jmx and jdk_management tests all pass after this change (no
regressions). There is also a new JTREG test which fails prior this
change and passes after. Updates to the diagnostic command have been
tested manually.

I understand that, if approved, the JDK and hotspot changes should get
pushed together? Hotspot changes are fairly trivial since it's only a
doc-update for the new JDK property in the relevant diagnostic command.

Could someone please review and sponsor this change? Please let me know
if there are questions.

Thanks,
Severin

[9] RFR: 8140649: imageFile should use delete[] with new[]

[Artem Smotrakov]

Hello,

Please review this small fix for jdk9/dev repo.

It updates imageFile.cpp to use delete[] operator together with new[]. 
It also adds a check to ImageLocation::set_data(u1*) method to prevent a 
possible null-dereference.


Bug: https://bugs.openjdk.java.net/browse/JDK-8140649
Webrev: 
http://cr.openjdk.java.net/~asmotrak/image_file_new_delete/webrev.01/


Artem

Re: Proposed API for JEP 259: Stack-Walking API

[David M. Lloyd]

I also agree - when an object type "passes through" a method, it's 
usually best to use an invariant type variable.


On 11/02/2015 06:44 AM, Paul Sandoz wrote:

I agree with Maurizio, the first signature is good enough.

One could argue that it might be better to apply PECS since it would encourage 
more consistent usage when it is actually required as all too often it’s easy 
to forget, and then too late to change. However, i don’t want to encourage the 
use of BaseStream since it was an unfortunate mistake that this made public.

Paul.


On 2 Nov 2015, at 13:26, Maurizio Cimadamore  
wrote:

So, we have three potential signatures here:

 T walk(Function function) //1

 T walk(Function function) //2

 T walk(Function function) 
//3


Under normal conditions (i.e. lambda parameter being passed to 'walk') I think 
all these signatures are fundamentally equivalent; (2) and (3) seem to have 
been designed for something like this:

Number n = walk(s -> new Integer(1));

That is, the function returns something that is more specific w.r.t. what is 
expected in the return type of walk. But - in reality, if 'walk' returns an R 
that is a subtype of T (as in the third signature), then walk also returns a T 
(as R is a subtype of T), so the result value can be passed where a T is 
expected.

The converse example:

Integer n1 = walk(s -> (Number)null);

Similarly fails on all three signatures.


More generally, with all such signatures, T will always receive:

* lower bound(s) (from the return value(s) of the lambda passed as a parameter 
to the 'walk' method)
* one upper bound (from the target-type associated with the 'walk' call.

Under such conditions, the upper bound will always be disregarded in favor of 
the lower bounds - meaning that instantiation of T will always be driven by 
what's inside the lambda. Signature (3) mentions different variables (R and T) 
but the end result is the same - as the bound says R extends T - meaning that 
lower bounds of R are propagated to T - leading to exactly the same situation.


In other words, I don't think there are obvious examples in which the behavior of 
these three signatures will be significantly different - if the return type of 'walk' 
would have been a generic type such as List, the situation would have been 
completely different - with with a 'naked' T, I think the first signature is good 
enough, and the third is, in my opinion, making things harder than what they need to 
be.

I think the second signature is not necessary, from a pure type-system 
perspective; but I guess one could make an argument for it, but purely in terms 
of consistency with other areas (after all, the second type-parameter of a 
Function is in a covariant position).

I hope this helps.

Maurizio



--
- DML

Re: Proposed API for JEP 259: Stack-Walking API

[Jason Mehrens]

Mandy,

Thread.dumpStack should generate the stacktrace elements then capture 
System.err into a local var and lock it while writing the output.  That would 
be compatible with what was done before.

Jason


From: core-libs-dev  on behalf of Mandy 
Chung 
Sent: Friday, October 30, 2015 2:04 PM
To: core-libs-dev
Subject: Proposed API for JEP 259: Stack-Walking API

JEP 259:  http://openjdk.java.net/jeps/259

Javadoc for the proposed StackWalker API:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

A simple way to walk the stack:

   StackWalker walker = new StackWalker(StackWalker.Option.CLASS_REFERENCE);
   walker.walk((s) ->  s.filter(f -> 
interestingClasses.contains(f.getDeclaringClass())).findFirst());

The current usage of sun.reflect.Reflection.getCallerClass(int depth) can be 
replaced with this StackWalker API.

Any feedback on the proposed API is appreciated.

Mandy

P.S. webrev of the current implementation:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/

Re: jmx-dev RFR 6425769: jmx remote bind address

[Jaroslav Bachorik]

Hi,

On 2.11.2015 16:19, Daniel Fuchs wrote:

Hi Severin,

Adding serviceability-...@openjdk.java.net into the loop - that's
a better alias than hotspot-dev for this kind of changes - maybe
someone from serviceability-dev will offer to sponsor :-)

I will let serviceability team members comment on the hotspot changes.

ConnectorBootstrap.java

I have one suggestion and one concern:

Suggestion: I would suggest to reuse 'csf' (Client Socket Factory) and
ssf  (Server Socket Factory) variables rather than introduce the two
new variables rmiServerSocketFactory and rmiClientSocketFactory.
You might want to create a new boolean 'useSocketFactory' variable,
if that makes the code more readable.

Concern: If I understand correctly how RMI socket factories work,
the client socket factory will be serialized and sent to the client
side. This is problematic for interoperability, as the class may not
present in the remote client - if the remote client is e.g. jdk 8.

As far as I can see, your new DefaultClientSocketFactory doesn't do
anything useful - so I would suggest to simply get rid of it, and only
set the Server Socket Factory when SSL is not involved.

Tests:

Concerning the tests - we're trying to get rid of shell scripts
rather than introducing new ones :-)
Could the test be rewritten in pure java using the Process API?

I believe there's even a test library that will let you do that
easily jdk/test/lib/testlibrary/jdk/testlibrary/
(see ProcessTools.java)

Other:

Also - I believe the new option should be documented in:
src/java.management/share/conf/management.properties

I share Daniel's concerns. Also, the part of the changeset is related to 
javax.rmi.ssl - someone maintaining this library should also comment here.

Also, the change is introducing new API (system property) and changing the 
existing one (adding SslRmiServerSocketFactory public constructors) so 
compatibility review process will have to be involved.

-JB-


best regards,

-- daniel

On 02/11/15 11:38, Severin Gehwolf wrote:
> Hi,
>
> Here is a patch addressing JDK-6425769. The issue is that the JMX agent
> binds to the wildcard address by default, preventing users to use
> system properties for JMX agents on multi-homed hosts. Given a host
> with local network interfaces, 192.168.0.1 and 192.168.0.2 say, it's
> impossible to start two JMX agents binding to fixed ports but to
> different network interfaces, 192.168.0.1:{9111,9112} and
> 192.168.0.2:{9111,9112} say.
>
> The JDK would bind to all local interfaces by default. In the above
> example to 192.168.0.1 *and* 192.168.0.2. The effect is that the second
> Java process would get a "Connection refused" error because another
> process has already been bound to the specified JMX/RMI port pairs.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-6425769
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/00/
>
> Testing done:
> jdk_jmx and jdk_management tests all pass after this change (no
> regressions). There is also a new JTREG test which fails prior this
> change and passes after. Updates to the diagnostic command have been
> tested manually.
>
> I understand that, if approved, the JDK and hotspot changes should get
> pushed together? Hotspot changes are fairly trivial since it's only a
> doc-update for the new JDK property in the relevant diagnostic command.
>
> Could someone please review and sponsor this change? Please let me know
> if there are questions.
>
> Thanks,
> Severin
>

Re: Proposed API for JEP 259: Stack-Walking API

[Mandy Chung]

> On Oct 31, 2015, at 11:05 PM, Jeroen Frijters  wrote:
> 
> Hi Mandy,
> 
> I like the API shape and this capability is very much needed. Thanks!
> 
> It does bring something related to mind and that is that the current Java 
> ecosystem is already very dependent on accurate stack frames (i.e. not 
> removing them when inlining) and with this API this will probably only become 
> worse. So it might be worth considering making this a hard spec requirement.
> 

How does it make it worse?  Would such a hard requirement prohibit any future 
optimization/performance improvement?

Mandy

> Regards,
> Jeroen
> 
>> -Original Message-
>> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On
>> Behalf Of Mandy Chung
>> Sent: Friday, October 30, 2015 20:05
>> To: core-libs-dev 
>> Subject: Proposed API for JEP 259: Stack-Walking API
>> 
>> JEP 259:  http://openjdk.java.net/jeps/259
>> 
>> Javadoc for the proposed StackWalker API:
>> 
>> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker
>> .html
>> 
>> A simple way to walk the stack:
>> 
>>   StackWalker walker = new
>> StackWalker(StackWalker.Option.CLASS_REFERENCE);
>>   walker.walk((s) ->  s.filter(f ->
>> interestingClasses.contains(f.getDeclaringClass())).findFirst());
>> 
>> The current usage of sun.reflect.Reflection.getCallerClass(int depth)
>> can be replaced with this StackWalker API.
>> 
>> Any feedback on the proposed API is appreciated.
>> 
>> Mandy
>> 
>> P.S. webrev of the current implementation:
>>   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
>> 
>> 
>> 
>> 
>> 
>> 
> 

Re: jmx-dev RFR 6425769: jmx remote bind address

[Severin Gehwolf]

Hi,

Thanks Jaroslav and Daniel for the reviews! Comments inline.

On Mon, 2015-11-02 at 16:54 +0100, Jaroslav Bachorik wrote:
> Hi,
> 
> On 2.11.2015 16:19, Daniel Fuchs wrote:
> > Hi Severin,
> > 
> > Adding serviceability-...@openjdk.java.net into the loop - that's
> > a better alias than hotspot-dev for this kind of changes - maybe
> > someone from serviceability-dev will offer to sponsor :-)
> > 
> > I will let serviceability team members comment on the hotspot
> > changes.
> > 
> > ConnectorBootstrap.java
> > 
> > I have one suggestion and one concern:
> > 
> > Suggestion: I would suggest to reuse 'csf' (Client Socket Factory)
> > and
> > ssf  (Server Socket Factory) variables rather than introduce the
> > two
> > new variables rmiServerSocketFactory and rmiClientSocketFactory.
> > You might want to create a new boolean 'useSocketFactory' variable,
> > if that makes the code more readable.
> > 
> > Concern: If I understand correctly how RMI socket factories work,
> > the client socket factory will be serialized and sent to the client
> > side. This is problematic for interoperability, as the class may
> > not
> > present in the remote client - if the remote client is e.g. jdk 8.
> > 
> > As far as I can see, your new DefaultClientSocketFactory doesn't do
> > anything useful - so I would suggest to simply get rid of it, and
> > only
> > set the Server Socket Factory when SSL is not involved.

Thanks. Fixed in updated webrev.

> > Tests:
> > 
> > Concerning the tests - we're trying to get rid of shell scripts
> > rather than introducing new ones :-)
> > Could the test be rewritten in pure java using the Process API?
> > 
> > I believe there's even a test library that will let you do that
> > easily jdk/test/lib/testlibrary/jdk/testlibrary/
> > (see ProcessTools.java)

It'll take me a bit to rewrite the test in pure Java, but should be
fine. This is not yet fixed in the updated webrev.

> > Other:
> > 
> > Also - I believe the new option should be documented in:
> > src/java.management/share/conf/management.properties

Docs have been updated
in src/java.management/share/conf/management.properties.

> I share Daniel's concerns. Also, the part of the changeset is related
> to javax.rmi.ssl - someone maintaining this library should also
> comment here.
> 
> Also, the change is introducing new API (system property) and
> changing the existing one (adding SslRmiServerSocketFactory public
> constructors) so compatibility review process will have to be
> involved.

OK. What exactly is there for me to do? I'm not familiar with this
process. Note that the intent would be to get this backported to JDK 8.

New webrev at:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/01/

Thanks,
Severin

> -JB-
> > 
> > best regards,
> > 
> > -- daniel
> > 
> > On 02/11/15 11:38, Severin Gehwolf wrote:
> > > Hi,
> > > 
> > > Here is a patch addressing JDK-6425769. The issue is that the JMX
> > > agent
> > > binds to the wildcard address by default, preventing users to use
> > > system properties for JMX agents on multi-homed hosts. Given a
> > > host
> > > with local network interfaces, 192.168.0.1 and 192.168.0.2 say,
> > > it's
> > > impossible to start two JMX agents binding to fixed ports but to
> > > different network interfaces, 192.168.0.1:{9111,9112} and
> > > 192.168.0.2:{9111,9112} say.
> > > 
> > > The JDK would bind to all local interfaces by default. In the
> > > above
> > > example to 192.168.0.1 *and* 192.168.0.2. The effect is that the
> > > second
> > > Java process would get a "Connection refused" error because
> > > another
> > > process has already been bound to the specified JMX/RMI port
> > > pairs.
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-6425769
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/
> > > 00/
> > > 
> > > Testing done:
> > > jdk_jmx and jdk_management tests all pass after this change (no
> > > regressions). There is also a new JTREG test which fails prior
> > > this
> > > change and passes after. Updates to the diagnostic command have
> > > been
> > > tested manually.
> > > 
> > > I understand that, if approved, the JDK and hotspot changes
> > > should get
> > > pushed together? Hotspot changes are fairly trivial since it's
> > > only a
> > > doc-update for the new JDK property in the relevant diagnostic
> > > command.
> > > 
> > > Could someone please review and sponsor this change? Please let
> > > me know
> > > if there are questions.
> > > 
> > > Thanks,
> > > Severin
> > > 
> > 
> 

Re: RFR (M) 8140802 - Clean up and refactor of class loading code for CDS

[Ioi Lam]

On 10/30/15 1:23 PM, Jiangli Zhou wrote:

Hi Ioi,

The change looks pretty clean. The new 
src/share/vm/classfile/systemDictionary_ext.hpp and 
src/share/vm/classfile/vmSymbols_ext.hpp do not have copyright header. Please 
add the copyright headers. Please also fix the copyright year for the modified 
files prior to pushing.
I added the missing heads and fixed the copyright year for all modified 
files.

In src/share/vm/classfile/sharedPathsMiscInfo.hpp, is the new “#include 
“classify/classLoader.hpp” necessary?

Yes, it's necessary for this part of the file

  static void trace_class_path(const char* msg, const char* name = NULL) {
ClassLoader::trace_class_path(msg, name);
  }

I am not sure why it wasn't detected by the C compiler before, but I did 
have a build failure when building without precompiled headers.


Thanks
- Ioi


I agree with the comments from Colleen and Karen regarding the usage of 
new_permanent_symbol() and TempNewSymbol in classLoaderExt.cpp. It doesn’t seem 
to be consistent.

Thanks,
Jiangli


On Oct 30, 2015, at 10:00 AM, Ioi Lam  wrote:

Please review the following fix:

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/

Bug: Clean up and refactor of class loading code for CDS

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

Summary of fix:

We need to clean up and refactor the class loading code in order
to support CDS in JDK9

[1] Remove code that has been made obsolete by the module changes
(such as supporting code used for meta-index file)
[2] Add new whitebox API to be used by CDS-related tests.
[3] Refactor the parsing of classlist files for future enhancements.
[4] Add new APIs in the class loading code to support Oracle CDS 
enhancements.

Tests:

JPRT
RBT - with same set of tests as hs-rt nightly

Thanks
- Ioi

Re: RFR (M) 8140802 - Clean up and refactor of class loading code for CDS

[Coleen Phillimore]

On 11/2/15 1:57 PM, Ioi Lam wrote:



On 10/30/15 1:31 PM, Coleen Phillimore wrote:



On 10/30/15 4:18 PM, Karen Kinnear wrote:

Coleen,

Question for you below please ...
On Oct 30, 2015, at 3:44 PM, Coleen Phillimore 
 wrote:



Hi Ioi,
This is a manageable code change.

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classListParser.hpp.html 



You forward declare Klass* but don't use it in this header file.
Also can you add a comment to #endif  to say what it's endifing. 
ie. // SHARE_VM_MEMORY_CLASSLISTPARSER_HPP


http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classLoaderExt.cpp.html 



  33   TempNewSymbol class_name_symbol = 
SymbolTable::new_permanent_symbol(parser->current_class_name(), 
THREAD);


This doesn't make sense.   If you want a permanent symbol, it 
doesn't need to get un-reference counted with the TempNewSymbol 
destructor.
Thank you for chiming in on this one - I wanted your opinion here. 
(this code used to be in MetaspaceShared::

preload_and_dump and I think was wrong there as well).
My understanding is that it is a TempNewSymbol that he wants, so he 
should call SymbolTable::new_symbol.
The code creates a (temporary) symbol for the name, and then calls 
SystemDictionary::resolve_or_null, which if it
succeeds will walk through the classFileParser which will create a 
permanent symbol for the class name,
via the symbol_alloc_batch handling. That would be consistent with 
the TempNewSymbol call in
SystemDictionary::resolve_or_null as well as in parse_stream - all 
places dealing with the class name

before doing classfile parsing.

Does that make sense?


Yes, this makes sense.  The symbol shouldn't be permanent.   Ioi can 
test this by putting a strange long name in the classlist file and 
make sure it doesn't make it out to the shared archive, since I think 
-Xshare:dump cleans out the SymbolTable before dumping.


After changing to use new_symbol instead of new_permanent_symbol, I 
ran -Xshare:dump:


$ java -Xshare:dump
Preload Warning: Cannot find sun/text/normalizer/UnicodeMatcher
...
total   :  13063134 [100.0% of total] out of  27385856 bytes [47.7% used]

$ strings images/jdk/lib/i386/hotspot/classes.jsa | grep 
sun/text/normalizer/UnicodeMatcher

sun/text/normalizer/UnicodeMatcher

So the unused symbols are not removed. Anyway, I have filed a separate 
issue for this: https://bugs.openjdk.java.net/browse/JDK-8141207


I just realized that the reason that it doesn't clean out unused symbols 
is that it would leave gaps in the Metaspace memory where they're 
stored, so there wouldn't be much point.


Coleen



Thanks
- Ioi



Coleen


thanks,
Karen

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/systemDictionary.cpp.udiff.html 



+// Make sure we have an entry in the SystemDictionary on success


This assert code is a copy of some code elsewhere.  Can you make it 
a function that they boh can call?

Can you also comment the raw #endif's to what they're endifing?

Otherwise, this looks okay.

Coleen


On 10/30/15 1:00 PM, Ioi Lam wrote:

Please review the following fix:

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/

Bug: Clean up and refactor of class loading code for CDS

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

Summary of fix:

We need to clean up and refactor the class loading code in order
to support CDS in JDK9

[1] Remove code that has been made obsolete by the module changes
(such as supporting code used for meta-index file)
[2] Add new whitebox API to be used by CDS-related tests.
[3] Refactor the parsing of classlist files for future 
enhancements.
[4] Add new APIs in the class loading code to support Oracle 
CDS enhancements.


Tests:

JPRT
RBT - with same set of tests as hs-rt nightly

Thanks
- Ioi

Re: RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization

[Mandy Chung]

> On Oct 20, 2015, at 11:28 AM, Roger Riggs  wrote:
> 
> Updated Javadoc:
>  http://cr.openjdk.java.net/~rriggs/cleaner-doc/

I’m happy with this API to provide an easy way to migrate away from finalizers. 
 Some thoughts:
1. For an existing finalizer (say cleanup function), it will be replaced with
 Cleaner.create().phantomCleanable(this, this::cleanup);

It may be useful to provide a sharable Cleaner instance that doesn’t go away.

The primary use for this Cleaner API is to replace finalizers.  I wonder it 
worths having a convenient method easier to relate to finalizer (say 
Cleaner.finalizable?) 

2. Do you know any existing use of soft / weak references in the JDK can 
benefit from this automatic cleanup mechanism? The typical cache implementation 
using soft/weak references is to purge the entries when the cache is accessed 
and manages the reference queue without a background thread.

I’m in two minds that it sounds sensible for this Cleaner API to extend for 
soft/weak references whereas I am not certain of the use cases.   Anyone can 
share the known use cases that would be helpful.

> 
> Updated Webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/


  57 final PhantomCleanable phantomCleanableList;
  59 final WeakCleanable weakCleanableList;
  61 final SoftCleanable softCleanableList;

- is there any benefit of keeping separate list for each reference type?  Each 
cleaner has one single ReferenceRueue and it seems that keeping one single 
Cleanable list could achieve similar performance in most cases while the 
implementation might be simplified.


 111 thread.setPriority(Thread.MAX_PRIORITY);

VM threads are near max priority.  The reference handler thread is MAX_PRORITY 
whereas the finalizer thread is MAX_PRIORITY-2.   I think the cleaner thread 
should not be of MAX_PRIORITY and also perhaps the caller may need a way to get 
the thread associated with a Cleaner so that users can set the priority if 
needed?

Mandy

[8u-dev] Request for review and approval: 8139863: [TESTBUG] Need to port tests for JDK-8134903 to 8u-dev

[Michael Haupt]

(Apologies for multiple posts with wrong subject lines.)

> Dear all,
> 
> cross-posted to jdk8u-dev and core-libs-dev.
> 
> Please review this change.
> Issue: https://bugs.openjdk.java.net/browse/JDK-8139863 
> 
> Webrev: http://cr.openjdk.java.net/~mhaupt/8139863/webrev.00 
> 
> 
> This adds a missing test to 8u-dev that was not pushed along with the feature 
> backport it belongs to. The backport was approved; see 
> http://mail.openjdk.java.net/pipermail/jdk8u-dev/2015-September/004185.html 
> .
> 
> Thanks,
> 
> Michael

Re: [9] RFR: 8140649: imageFile should use delete[] with new[]

[Jim Laskey (Oracle)]

+1

> On Nov 2, 2015, at 9:30 AM, Artem Smotrakov  
> wrote:
> 
> Hello,
> 
> Please review this small fix for jdk9/dev repo.
> 
> It updates imageFile.cpp to use delete[] operator together with new[]. It 
> also adds a check to ImageLocation::set_data(u1*) method to prevent a 
> possible null-dereference.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8140649
> Webrev: http://cr.openjdk.java.net/~asmotrak/image_file_new_delete/webrev.01/
> 
> Artem

Re: RFR (M) 8140802 - Clean up and refactor of class loading code for CDS

[Ioi Lam]

On 10/30/15 12:44 PM, Coleen Phillimore wrote:


Hi Ioi,
This is a manageable code change.

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classListParser.hpp.html

You forward declare Klass* but don't use it in this header file.
Also can you add a comment to #endif  to say what it's endifing. ie. 
// SHARE_VM_MEMORY_CLASSLISTPARSER_HPP



Fixed.

I also changed ClassListParser to a StackObj as you suggested in another 
e-mail to me.



http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classLoaderExt.cpp.html
   33   TempNewSymbol class_name_symbol = 
SymbolTable::new_permanent_symbol(parser->current_class_name(), THREAD);
This doesn't make sense.   If you want a permanent symbol, it doesn't 
need to get un-reference counted with the TempNewSymbol destructor.


I changed it to

  TempNewSymbol class_name_symbol = 
SymbolTable::new_symbol(parser->current_class_name(), THREAD);




http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/systemDictionary.cpp.udiff.html

+// Make sure we have an entry in the SystemDictionary on success
I will actually remove this block of code in the next update to make 
things simpler.


This assert code is a copy of some code elsewhere.  Can you make it a 
function that they boh can call?

Can you also comment the raw #endif's to what they're endifing?


Will do.

Thanks


Otherwise, this looks okay.

Coleen


On 10/30/15 1:00 PM, Ioi Lam wrote:

Please review the following fix:

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/

Bug: Clean up and refactor of class loading code for CDS

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

Summary of fix:

We need to clean up and refactor the class loading code in order
to support CDS in JDK9

[1] Remove code that has been made obsolete by the module changes
(such as supporting code used for meta-index file)
[2] Add new whitebox API to be used by CDS-related tests.
[3] Refactor the parsing of classlist files for future enhancements.
[4] Add new APIs in the class loading code to support Oracle CDS 
enhancements.


Tests:

JPRT
RBT - with same set of tests as hs-rt nightly

Thanks
- Ioi

Re: RFR [9] 8140687: Move @Contended to the jdk.internal.vm.annotation package

[Paul Sandoz]

> On 2 Nov 2015, at 00:56, Chris Hegarty  wrote:
> 
> In line with the intended location for other VM annotations,
> see 8138732 [1], @sun.misc.Contended should be moved
> to the jdk.internal.vm.annotation package.
> 
> http://cr.openjdk.java.net/~chegar/8140687/00/
> 

+1

Paul.

Re: RFR [9] 8140606: Update library code to use internal Unsafe

[Paul Sandoz]

> On 28 Oct 2015, at 20:55, Chris Hegarty  wrote:
> 
> Following on from 8139891 "Prepare Unsafe for true encapsulation” [1],
> the JDK library code should use the internal Unsafe class, and not
> sun.misc.Unsafe.
> 
> http://cr.openjdk.java.net/~chegar/8140606/00/
> 

+1

Paul.

> This will be pushed to jdk9/dev once 8139891 makes its way from
> hs-rt.
> 
> -Chris.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8139891

Re: RFR [9] 8140687: Move @Contended to the jdk.internal.vm.annotation package

[Alan Bateman]

On 01/11/2015 23:56, Chris Hegarty wrote:

In line with the intended location for other VM annotations,
see 8138732 [1], @sun.misc.Contended should be moved
to the jdk.internal.vm.annotation package.

http://cr.openjdk.java.net/~chegar/8140687/00/

-Chris.

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

Looks good.

-Alan.

Re: RFR (M) 8140802 - Clean up and refactor of class loading code for CDS

[Ioi Lam]

On 10/30/15 1:31 PM, Coleen Phillimore wrote:



On 10/30/15 4:18 PM, Karen Kinnear wrote:

Coleen,

Question for you below please ...
On Oct 30, 2015, at 3:44 PM, Coleen Phillimore 
 wrote:



Hi Ioi,
This is a manageable code change.

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classListParser.hpp.html 



You forward declare Klass* but don't use it in this header file.
Also can you add a comment to #endif  to say what it's endifing. ie. 
// SHARE_VM_MEMORY_CLASSLISTPARSER_HPP


http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classLoaderExt.cpp.html 



  33   TempNewSymbol class_name_symbol = 
SymbolTable::new_permanent_symbol(parser->current_class_name(), 
THREAD);


This doesn't make sense.   If you want a permanent symbol, it 
doesn't need to get un-reference counted with the TempNewSymbol 
destructor.
Thank you for chiming in on this one - I wanted your opinion here. 
(this code used to be in MetaspaceShared::

preload_and_dump and I think was wrong there as well).
My understanding is that it is a TempNewSymbol that he wants, so he 
should call SymbolTable::new_symbol.
The code creates a (temporary) symbol for the name, and then calls 
SystemDictionary::resolve_or_null, which if it
succeeds will walk through the classFileParser which will create a 
permanent symbol for the class name,
via the symbol_alloc_batch handling. That would be consistent with 
the TempNewSymbol call in
SystemDictionary::resolve_or_null as well as in parse_stream - all 
places dealing with the class name

before doing classfile parsing.

Does that make sense?


Yes, this makes sense.  The symbol shouldn't be permanent.   Ioi can 
test this by putting a strange long name in the classlist file and 
make sure it doesn't make it out to the shared archive, since I think 
-Xshare:dump cleans out the SymbolTable before dumping.


After changing to use new_symbol instead of new_permanent_symbol, I ran 
-Xshare:dump:


$ java -Xshare:dump
Preload Warning: Cannot find sun/text/normalizer/UnicodeMatcher
...
total   :  13063134 [100.0% of total] out of  27385856 bytes [47.7% used]

$ strings images/jdk/lib/i386/hotspot/classes.jsa | grep 
sun/text/normalizer/UnicodeMatcher

sun/text/normalizer/UnicodeMatcher

So the unused symbols are not removed. Anyway, I have filed a separate 
issue for this: https://bugs.openjdk.java.net/browse/JDK-8141207


Thanks
- Ioi



Coleen


thanks,
Karen

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/systemDictionary.cpp.udiff.html 



+// Make sure we have an entry in the SystemDictionary on success


This assert code is a copy of some code elsewhere.  Can you make it 
a function that they boh can call?

Can you also comment the raw #endif's to what they're endifing?

Otherwise, this looks okay.

Coleen


On 10/30/15 1:00 PM, Ioi Lam wrote:

Please review the following fix:

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/

Bug: Clean up and refactor of class loading code for CDS

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

Summary of fix:

We need to clean up and refactor the class loading code in order
to support CDS in JDK9

[1] Remove code that has been made obsolete by the module changes
(such as supporting code used for meta-index file)
[2] Add new whitebox API to be used by CDS-related tests.
[3] Refactor the parsing of classlist files for future 
enhancements.
[4] Add new APIs in the class loading code to support Oracle 
CDS enhancements.


Tests:

JPRT
RBT - with same set of tests as hs-rt nightly

Thanks
- Ioi

Re: RFR [9] 8140687: Move @Contended to the jdk.internal.vm.annotation package

[Aleksey Shipilev]

On 11/02/2015 02:56 AM, Chris Hegarty wrote:
> In line with the intended location for other VM annotations,
> see 8138732 [1], @sun.misc.Contended should be moved
> to the jdk.internal.vm.annotation package.
> 
> http://cr.openjdk.java.net/~chegar/8140687/00/

Changes look good to me.

But what's the compatibility story for JSR 166? With this move, we would
be unable to bootclasspath JSR 166 JAR, since it references the "old"
sun.misc.Contended?

Thanks,
-Aleksey

Re: Proposed API for JEP 259: Stack-Walking API

[Maurizio Cimadamore]

So, we have three potential signatures here:

 T walk(Function function) //1

 T walk(Function 
function) //2


 T walk(Function 
function) //3



Under normal conditions (i.e. lambda parameter being passed to 'walk') I 
think all these signatures are fundamentally equivalent; (2) and (3) 
seem to have been designed for something like this:


Number n = walk(s -> new Integer(1));

That is, the function returns something that is more specific w.r.t. 
what is expected in the return type of walk. But - in reality, if 'walk' 
returns an R that is a subtype of T (as in the third signature), then 
walk also returns a T (as R is a subtype of T), so the result value can 
be passed where a T is expected.


The converse example:

Integer n1 = walk(s -> (Number)null);

Similarly fails on all three signatures.


More generally, with all such signatures, T will always receive:

* lower bound(s) (from the return value(s) of the lambda passed as a 
parameter to the 'walk' method)

* one upper bound (from the target-type associated with the 'walk' call.

Under such conditions, the upper bound will always be disregarded in 
favor of the lower bounds - meaning that instantiation of T will always 
be driven by what's inside the lambda. Signature (3) mentions different 
variables (R and T) but the end result is the same - as the bound says R 
extends T - meaning that lower bounds of R are propagated to T - leading 
to exactly the same situation.



In other words, I don't think there are obvious examples in which the 
behavior of these three signatures will be significantly different - if 
the return type of 'walk' would have been a generic type such as 
List, the situation would have been completely different - with with 
a 'naked' T, I think the first signature is good enough, and the third 
is, in my opinion, making things harder than what they need to be.


I think the second signature is not necessary, from a pure type-system 
perspective; but I guess one could make an argument for it, but purely 
in terms of consistency with other areas (after all, the second 
type-parameter of a Function is in a covariant position).


I hope this helps.

Maurizio

On 01/11/15 10:11, Timo Kinnunen wrote:

I’m not quite sure I follow you, so my apologies if I misunderstood you. What 
I’m trying to point out is that changing the type from “T” to “? extends T”  
doesn’t add any expressiveness in this method. These two signatures are both 
equally expressive for users of this API:

public  T walk(Function function)
public  T walk(Function 
function)

Maybe this is easier to see if we give a name to our wildcard type:

public  T walk(Function 
function)
public  T walk(Function function)

Clearly these two are equivalent, we just now have a real name for “? extends 
T” is, it is “R”. For a user who is calling this method, what is gained by 
having it return the potentially less specific type “T” that could be some 
super type of “R” and not have it just return the type “R”? The implementation 
of walk doesn’t need them, it can be done without bringing in extra types from 
R’s type the hierarchy.

I don’t think there is any gain. For all types  where A extends B, B 
extends C, the method taking A and giving B to the user storing it to C gives a 
transformation A→B→C. For this case we have two transformation corresponding to the 
two alternative methods:

In “ R walk(Function function)”,
R→R→T is available, because R extends T so R can be stored in T.
In “ T walk(Function f)”,  
R→T→T is available, because well, T is a T.

Changing a single type parameter in an API from “T” to “? extends T” in a wrong 
place can have a dramatic effect on the quality of type inference results that 
the users receive when using the API. The affects functional programming style 
much, much more than it does a more procedural style. So I want to make it 
clear now that for most users a single additional wildcard in the API is not a 
disaster. For people doing more functional programming though, the effects can 
feel punishing as they have to add more casts, type arguments or extra local 
variables to make more types explicit --- which type inference would have 
otherwise inferred --- had the wildcard not been present.

This is why I think it’s prudent that the expected benefits of adding any one 
new wildcard are stated clearly and in practical terms. These benefits can then 
be evaluated against the drawbacks, and an informed decision can be made. Which 
is why I would like to ask:

- What are the expected benefits that arise from using a wildcard type in this 
method rather than a non-wildcard type, and
- How will this manifest in practice in the code (i.e. what is one supposed to 
look for) to verify that the actual results match what we expected?


TIA!







Sent from Mail for 

Re: Proposed API for JEP 259: Stack-Walking API

[Paul Sandoz]

I agree with Maurizio, the first signature is good enough.

One could argue that it might be better to apply PECS since it would encourage 
more consistent usage when it is actually required as all too often it’s easy 
to forget, and then too late to change. However, i don’t want to encourage the 
use of BaseStream since it was an unfortunate mistake that this made public.

Paul.

> On 2 Nov 2015, at 13:26, Maurizio Cimadamore  
> wrote:
> 
> So, we have three potential signatures here:
> 
>  T walk(Function function) //1
> 
>  T walk(Function function) //2
> 
>  T walk(Function function) 
> //3
> 
> 
> Under normal conditions (i.e. lambda parameter being passed to 'walk') I 
> think all these signatures are fundamentally equivalent; (2) and (3) seem to 
> have been designed for something like this:
> 
> Number n = walk(s -> new Integer(1));
> 
> That is, the function returns something that is more specific w.r.t. what is 
> expected in the return type of walk. But - in reality, if 'walk' returns an R 
> that is a subtype of T (as in the third signature), then walk also returns a 
> T (as R is a subtype of T), so the result value can be passed where a T is 
> expected.
> 
> The converse example:
> 
> Integer n1 = walk(s -> (Number)null);
> 
> Similarly fails on all three signatures.
> 
> 
> More generally, with all such signatures, T will always receive:
> 
> * lower bound(s) (from the return value(s) of the lambda passed as a 
> parameter to the 'walk' method)
> * one upper bound (from the target-type associated with the 'walk' call.
> 
> Under such conditions, the upper bound will always be disregarded in favor of 
> the lower bounds - meaning that instantiation of T will always be driven by 
> what's inside the lambda. Signature (3) mentions different variables (R and 
> T) but the end result is the same - as the bound says R extends T - meaning 
> that lower bounds of R are propagated to T - leading to exactly the same 
> situation.
> 
> 
> In other words, I don't think there are obvious examples in which the 
> behavior of these three signatures will be significantly different - if the 
> return type of 'walk' would have been a generic type such as List, the 
> situation would have been completely different - with with a 'naked' T, I 
> think the first signature is good enough, and the third is, in my opinion, 
> making things harder than what they need to be.
> 
> I think the second signature is not necessary, from a pure type-system 
> perspective; but I guess one could make an argument for it, but purely in 
> terms of consistency with other areas (after all, the second type-parameter 
> of a Function is in a covariant position).
> 
> I hope this helps.
> 
> Maurizio
> 

Re: Proposed API for JEP 259: Stack-Walking API

[David M. Lloyd]

On 10/30/2015 11:24 PM, Mandy Chung wrote:

On Oct 30, 2015, at 3:38 PM, David M. Lloyd  wrote:

(that's what I was looking for before), or maybe even ToIntFunction where X is 
something that might inform the next batch size based on the work that has already been 
done - maybe even a Stream that consists only of the current batch?

Or another idea:
   T walk(BiFunction 
function);

where the IntConsumer can be called at any time to update a minimum number of 
remaining frames needed, which in turn can (with the knowledge of how many 
elements have been consumed by the stream) inform the next batch size.


The stack walker won’t get the next batch until all frames of the current batch 
are consumed.  I think the current walk methods would cover what this one does. 
 There are other ideas to add other variants of the walk methods and short-cut 
for performance.I keep these ideas in mind and consider them once more 
feedback is collected.


I think there are two problems with the current approach:

1. The function for getting the next batch size is not coupled to the 
function actually doing the work.  I think it is just as likely (if not 
more so) that any information about the optimum number of frames to be 
gathered per batch can only be sensibly determined as a result of the 
current work being done.  In this case, you'd have to do something odd 
like use a captured shared AtomicInteger or similar to pass information 
back and forth, which is not good.


2. The fact that frames are acquired in batches is an implementation 
detail.  I do not believe it's a reasonable expectation of a user to 
know the optimum batch size; the API should not be exposing 
implementation details like this.  On the other hand, the user might be 
reasonably expected to be able to inform the stack walker as to how many 
more frames are expected to be needed.  The stack walker could easily 
use this information, even if it fetches in batches, by keeping a 
"remainingNeeded" counter of number of remaining frames expected to be 
used, which is decremented each time the stream returns an item, and 
read when a new batch is needed: it could (for example) use Math.min(10, 
remainingNeeded).


--
- DML