Re: RFR 8195650 Method references to VarHandle accessors

[Karen Kinnear]

Looks good. Matches the existing JVMS. Thanks for the tests.

thanks,
Karen

> On Jun 19, 2018, at 8:08 PM, Paul Sandoz  wrote:
> 
> Hi,
> 
> Please review the following fix to ensure method references to VarHandle 
> signature polymorphic methods are supported at runtime (specifically the 
> method handle to a signature polymorphic method can be loaded from the 
> constant pool):
> 
>  http://cr.openjdk.java.net/~psandoz/jdk/JDK-8195650-varhandle-mref/webrev/ 
> 
> 
> I also added a “belts and braces” test to ensure a constant method handle to 
> MethodHandle::invokeBasic cannot be loaded if outside of the j.l.invoke 
> package.
> 
> Paul.
> 

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[Karen Kinnear]

>> On 15/05/2018 10:52 AM, David Holmes wrote:
>>>>> This review is being spread across four groups: langtools, core-libs, 
>>>>> hotspot and serviceability. This is the specific review thread for 
>>>>> core-libs - webrev:
>>>>> 
>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
>>>>> 
>>>>> See below for full details - including annotated full webrev guiding the 
>>>>> review.
>>>>> 
>>>>> The intent is to have JEP-181 targeted and integrated by the end of this 
>>>>> month.
>>>>> 
>>>>> Thanks,
>>>>> David
>>>>> -
>>>>> 
>>>>> The nestmates project (JEP-181) introduces new classfile attributes to 
>>>>> identify classes and interfaces in the same nest, so that the VM can 
>>>>> perform access control based on those attributes and so allow direct 
>>>>> private access between nestmates without requiring javac to generate 
>>>>> synthetic accessor methods. These access control changes also extend to 
>>>>> core reflection and the MethodHandle.Lookup contexts.
>>>>> 
>>>>> Direct private calls between nestmates requires a more general calling 
>>>>> context than is permitted by invokespecial, and so the JVMS is updated to 
>>>>> allow, and javac updated to use, invokevirtual and invokeinterface for 
>>>>> private class and interface method calls respectively. These changed 
>>>>> semantics also extend to MethodHandle findXXX operations.
>>>>> 
>>>>> At this time we are only concerned with static nest definitions, which 
>>>>> map to a top-level class/interface as the nest-host and all its nested 
>>>>> types as nest-members.
>>>>> 
>>>>> Please see the JEP for further details.
>>>>> 
>>>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8046171
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8010319
>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
>>>>> 
>>>>> All of the specification changes have been previously been worked out by 
>>>>> the Valhalla Project Expert Group, and the implementation reviewed by the 
>>>>> various contributors and discussed on the valhalla-dev mailing list.
>>>>> 
>>>>> Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, 
>>>>> Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir 
>>>>> Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
>>>>> 
>>>>> Master webrev of all changes:
>>>>> 
>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
>>>>> 
>>>>> Annotated master webrev index:
>>>>> 
>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
>>>>> 
>>>>> Performance: this is expected to be performance neutral in a general 
>>>>> sense. Benchmarking and performance runs are about to start.
>>>>> 
>>>>> Testing Discussion:
>>>>> --
>>>>> 
>>>>> The testing for nestmates can be broken into four main groups:
>>>>> 
>>>>> -  New tests specifically related to nestmates and currently in the 
>>>>> runtime/Nestmates directory
>>>>> 
>>>>> - New tests to complement existing tests by adding in testcases not 
>>>>> previously expressible.
>>>>>-  For example java/lang/invoke/SpecialInterfaceCall.java tests use of 
>>>>> invokespecial for private interface methods and performing receiver 
>>>>> typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do 
>>>>> similar tests for invokeinterface.
>>>>> 
>>>>> -  New JVM TI tests to verify the spec changes related to nest attributes.
>>>>> 
>>>>> -  Existing tests significantly affected by the nestmates changes, 
>>>>> primarily:
>>>>> -  runtime/SelectionResolution
>>>>> 
>>>>> In most cases the nestmate changes makes certain invocations that 
>>>>> were illegal, legal (e.g. not requiring invokespecial to invoke private 
>>>>> interface methods; allowing access to private members via 
>>>>> reflection/Methodhandles that were previously not allowed).
>>>>> 
>>>>> - Existing tests incidentally affected by the nestmate changes
>>>>> 
>>>>>This includes tests of things utilising class 
>>>>> redefinition/retransformation to alter nested types but which 
>>>>> unintentionally alter nest relationships (which is not permitted).
>>>>> 
>>>>> There are still a number of tests problem-listed with issues filed 
>>>>> against them to have them adapted to work with nestmates. Some of these 
>>>>> are intended to be addressed in the short-term, while some (such as the 
>>>>> runtime/SelectionResolution test changes) may not eventuate.
>>>>> 
>>>>> - https://bugs.openjdk.java.net/browse/JDK-8203033
>>>>> - https://bugs.openjdk.java.net/browse/JDK-8199450
>>>>> - https://bugs.openjdk.java.net/browse/JDK-8196855
>>>>> - https://bugs.openjdk.java.net/browse/JDK-8194857
>>>>> - https://bugs.openjdk.java.net/browse/JDK-8187655
>>>>> 
>>>>> There is also further test work still to be completed (the JNI and JDI 
>>>>> invocation tests):
>>>>> - https://bugs.openjdk.java.net/browse/JDK-8191117
>>>>> which will continue in parallel with the main RFR.
>>>>> 
>>>>> Pre-integration Testing:
>>>>>   - General:
>>>>>  - Mach5: hs/jdk tier1,2
>>>>>  - Mach5: hs-nightly (tiers 1 -3)
>>>>>   - Targetted
>>>>> - nashorn (for asm changes)
>>>>> - hotspot: runtime/*
>>>>>serviceability/*
>>>>>compiler/*
>>>>>vmTestbase/*
>>>>> - jdk: java/lang/invoke/*
>>>>>java/lang/reflect/*
>>>>>java/lang/instrument/*
>>>>>java/lang/Class/*
>>>>>java/lang/management/*
>>>>>- langtools: tools/javac
>>>>> tools/javap
>>>>> 

Re: (M) RFR: 8200167: Validate more special case invocations

[Karen Kinnear]

Looks good!

Thank you so much David and Vladimir for all the work to cover the corner cases.

thanks,
Karen

> On Apr 26, 2018, at 12:12 AM, David Holmes  wrote:
> 
> Revised webrev: http://cr.openjdk.java.net/~dholmes/8200167/webrev.v2/
> 
> Karen formulated an additional test scenario invoking Object methods through 
> an interface reference, which when turned into a new testcase demonstrated 
> that the receiver typecheck was also missing in that case for Methodhandle's 
> from findSpecial.
> 
> Again Vladimir Ivanov formulated the fix for this. Thanks Vladimir!
> 
> Summary of changes from original webrev:
> 
> - We introduce a new version of DirectMethodHandle.preparedlambdaForm that 
> takes the caller class as a parameter, and that class is checked for being an 
> interface (not the method's declaring class) to trigger the switch to 
> LF_SPECIAL_IFC. (This obviously addresses one problem with invoking Object 
> methods - as Object is not an interface - but by itself did not fix the 
> problem.)
> 
> - We introduce a new LambdaForm kind, DIRECT_INVOKE_SPECIAL_IFC, which we use 
> when dealing with LF_INVSPECIAL_IFC. (This was the key in ensuring the 
> receiver typecheck via Special.checkReceiver remained in the generated code.)
> 
> - In the test we:
>  - introduce a new invokeDirect testcase for Object.toString(), but we need 
> to do that via a modified jcod file (as javac generates an invokevirtual)
>  - introduce the new invokeSpecialObjectMH(I2 i) call for the MH case.
> 
> Thanks,
> David
> 
> On 17/04/2018 6:48 PM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8200167
>> webrev: http://cr.openjdk.java.net/~dholmes/8200167/webrev/
>> Credit to John Rose and Vladimir Ivanov for the form of the MH fix; and to 
>> Tobias Hartmann for the C1 fix. Their help was very much appreciated (and 
>> needed!).
>> tl;dr version: there are some places where badly formed interface method 
>> calls (which are not detected by the verifier) were missing runtime checks 
>> on the type of the receiver. Similar issues have been fixed in the past and 
>> this was a remaining hole in relation to invokespecial semantics and 
>> interface calls via MethodHandles. It also turned out there was an issue in 
>> C1 that affected direct invokespecial calls.
>> Description of changes:
>> - src/java.base/share/classes/java/lang/invoke/MethodTypeForm.java
>> Added a new form LF_INVSPECIAL_IFC for invokespecial of interface methods.
>> - src/java.base/share/classes/java/lang/invoke/MethodHandles.java
>> Renamed callerClass parameter to boundCallerClass, where it originates from 
>> findBoundCallerClass. The name "callerClass" is misleading because most of 
>> the time it is NULL!
>> In getDirectMethodCommon, pass the actual caller class (lookupClass) to 
>> DirectMethodHandle.make. And correct the comment about restrictReceiver.
>> - src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java
>> DirectMethodHandle.make now takes a callerClass parameter (which may be 
>> null).
>> DirectMethodHandle make "receiver" parameter is renamed "refc" as it is not 
>> the receiver (it's the resolved type of the method holder ie REFC).
>> The Special subclass now takes a "checkClass" parameter which is either 
>> refc, or callerClass. This is what we will check the receiver against.
>> In preparedLambdaForm we switch from LF_INVSPECIAL to LF_INVSPECIAL_IFC if 
>> the target method is in an interface.
>> In makePreparedLambdaForm we augment the needsReceiverCheck test to include 
>> LF_INVSPECIAL_IFC. (So we will not be doing a new receiver type check on all 
>> invokespecials, just those involving interface methods.)
>> Added DMH.checkReceiver which is overridden by both the Special and 
>> Interface subclasses.
>> - src/hotspot/share/c1/c1_Canonicalizer.cpp
>> Don't optimize away the checkcast when it is an invokespecial receiver check.
>> - test/jdk/java/lang/invoke/SpecialInterfaceCall.java
>> (I've moved this test back and forth between hotspot/runtime and j.l.invoke 
>> as it spans direct calls and MH calls. But I think on balance it lands 
>> better here.)
>> The test sets up direct interface method calls for which invokespecial is 
>> generated by javac, and similarly it creates MethodHandles with 
>> invokespecial semantics. The should-not-throw cases are trivial. The 
>> should-throw cases involve MH wizardry.
>> The test is broken into three parts to check the behaviour on first use 
>> (when the method has to be initially resolved); on second use (to test we 
>> don't use the cpcache in a way that is invalid); and again after forcing JIT 
>> compilation of the calls.
>> The test is run 5 times to exercise the interpreter (yes the multiple runs 
>> internal to the test are redundant in this case, but it's much simpler to 
>> just run it all than try to work out what needs to be run); the three 
>> variants of using C1, plus a C2 only run.
>> ---
>> Testing:
>>   - the 

Re: RFR: 8200238: Reduce number of exceptions created when calling MemberName$Factory::resolveOrNull

[Karen Kinnear]

Claes,

Thank you for this fix. Glad it makes a startup difference. Agree with Lois.

thanks,
Karen

p.s. haven’t had a chance to trace the logic of your explanation below in a 
debugger - I don’t
quite follow it. But the fix makes sense.

> On Mar 27, 2018, at 11:57 AM, Lois Foltan <lois.fol...@oracle.com> wrote:
> 
> On 3/27/2018 2:49 AM, Claes Redestad wrote:
> 
>> 
>> 
>> On 2018-03-26 17:51, Claes Redestad wrote:
>>> Karen,
>>> 
>>> On 2018-03-26 17:15, Karen Kinnear wrote:
>>>> Claes,
>>>> 
>>>> Discussed with Lois. We think that it would make more sense to pass the 
>>>> new argument into MethodHandles::resolve_MemberName and at all three 
>>>> places that we currently CHECK_PENDING_EXCEPTION/return null there
>>>> - if speculative flag is set - CLEAR_PENDING_EXCEPTION before you 
>>>> return null
>>>> - and yes - do this for all three cases, not just the METHOD case
>>> 
>>> ok.
>> 
>> New webrev:
>> 
>> http://cr.openjdk.java.net/~redestad/8200238/open.01/
> 
> Hi Claes,
> Looks good.  One minor comment.
> 
> hotspot/share/prims/methodHandles.cpp:
> - line #1237.  Consider putting some explanation in the assert statement 
> instead of a blank string.  Something like "speculative resolve mode has 
> encountered an unexpected pending exception"
> 
> I don't need to see another webrev.
> 
> Thanks,
> Lois
> 
>> 
>> Thanks!
>> 
>> /Claes

Re: RFR: 8200238: Reduce number of exceptions created when calling MemberName$Factory::resolveOrNull

[Karen Kinnear]

Claes,

Discussed with Lois. We think that it would make more sense to pass the new 
argument into MethodHandles::resolve_MemberName and at all three places that we 
currently CHECK_PENDING_EXCEPTION/return null there
   - if speculative flag is set - CLEAR_PENDING_EXCEPTION before you return null
   - and yes - do this for all three cases, not just the METHOD case

Couple of questions though:
1) do you actually reach the new code in MHN_resolve_Mem? Could you possibly 
add an assertion there that
there is no exception pending and/or print the exception if there is one 
pending? 
With the CHECK_NULL in the call to resolve_MemberName I do not expect you to 
get here with a pending exception,
so the question arises as to when you would have a null resolved, but no 
pending exception?

2) with the change you just sent out - do you really get a performance 
improvement?
I’m confused about where that comes from

My apologies - I was incorrect - we are not converting Error -> Exception in 
MHN_resolve_Mem - so I
do not know why we are burying existing exceptions here - longer-term I think 
we need to clean this up
and as David points out - at least for the ResolveOrFail case - store the 
original exception as a cause.

thanks,
Karen

> On Mar 26, 2018, at 8:08 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> MethodHandleNatives::resolve are used by both 
> MemberName$Factory::resolveOrFail and ::resolveOrNull, the latter allowing 
> speculative lookup of methods.
> 
> While the linkResolver code this calls into in the VM will create and throw 
> exceptions for all cases where a method or field is missing, these are caught 
> by MethodHandles::resolve_MemberName and an empty handle is returned instead. 
> However, the outer method, MHN_resolve_Mem will always create a new exception 
> and throw it.
> 
> In case of resolveOrNull, we'll then ignore whatever exception is thrown in 
> the java code, so in effect we're creating and ignoring two exceptions on a 
> failed lookup.
> 
> We can cut this in half by passing a boolean to MethodHandleNatives::resolve 
> to indicate whether we're doing a speculative lookup (resolveOrNull) or not 
> (resolveOrFail):
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8200238
> 
> Webrev: http://cr.openjdk.java.net/~redestad/8200238/open.00/
> 
> This is a small startup optimization for applications that have a high miss 
> rate (looks up a lot of methods that doesn't exist in the pre-generated 
> Holders).
> 
> Thanks!
> 
> /Claes
> 

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

[Karen Kinnear]

Harold,

Looks good. Many thanks!

Karen

> On Feb 12, 2018, at 3:50 PM, harold seigel <harold.sei...@oracle.com> wrote:
> 
> Hi Karen,
> 
> Thanks for looking at this!
> 
> I re-ran the ParallelClassLoading tests with no options, with 
> -XX:+AllowParallelDefineClass, and with -XX:+AlwaysLockClassLoader, and all 
> the tests passed.  I also determined that the few Mach5 regression test 
> failures that I encountered were unrelated to my change.
> Please see this latest webrev that adds 12 as an expiration date, removes the 
> 'guarantee' section, and fixes the comment at line 786 of 
> systemDictionary.cpp.
> http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html 
> <http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html>
> Thanks, Harold
> 
> On 2/12/2018 2:39 PM, Karen Kinnear wrote:
>> Harold,
>> 
>> Thanks for doing this. 
>> 
>> I think you told me that
>> 1) the version change has made it in
>> 2) you also put 12 as an expiration date
>> 3) you are running the ParallelClassLoading tests with the remaining two 
>> flags (you’ve already
>> run them without any flags):
>>   AllowParallelDefineClass = true and AlwaysLockClassLoader=true
>> 
>> In terms of the guarantee in question
>>   // For UnsyncloadClass only
>>  848   // If they got a linkageError, check if a parallel class load 
>> succeeded.
>>  849   // If it did, then for bytecode resolution the specification 
>> requires
>>  850   // that we return the same result we did for the other thread, 
>> i.e. the
>>  851   // successfully loaded InstanceKlass
>>  852   // Should not get here for classloaders that support parallelism
>>  853   // with the new cleaner mechanism, even with 
>> AllowParallelDefineClass
>>  854   // Bootstrap goes through here to allow for an extra guarantee 
>> check
>>  855   if (UnsyncloadClass || (class_loader.is_null())) {
>>  856 if (k == NULL && HAS_PENDING_EXCEPTION
>>  857   && 
>> PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
>>  858   MutexLocker mu(SystemDictionary_lock, THREAD);
>>  859   InstanceKlass* check = find_class(d_hash, name, dictionary);
>>  860   if (check != NULL) {
>>  861 // Klass is already loaded, so just use it
>>  862 k = check;
>>  863 CLEAR_PENDING_EXCEPTION;
>>  864 guarantee((!class_loader.is_null()), "dup definition for 
>> bootstrap loader?");
>>  865   }
>>  866 }
>>  867   }
>> 
>>   1) I agree you can remove the entire section
>>   - the guarantee was there for future proofing in case we ever allowed 
>> parallel class loading of the
>> same class for the null loader and to make sure I didn’t have any 
>> logic holes.
>>   - I would not put an assertion for the first half of the condition - I 
>> would remove completely
>>   - the code currently prevents parallel class loading of the same class 
>> for the null loader at:
>> resolve_instance_class_or_null see line 785 …
>>   while (!class_has_been_loaded && old probe && old 
>> probe->instance_load_in_progress()) {
>>  //  case x: bootstrap class loader: prevent futile class loading,
>>  // wait on first requestor
>>  if (class_loader.is_null()) {
>> SystemDictionary_lock->wait();
>> 
>> This logic means that there is a registered INSTANCE_LOAD on this 
>> placeholder entry.
>> 
>> 
>> Other minor comments (sorry if you already got these and I missed them in 
>> earlier emails)
>> - all in SystemDictionary.cpp
>> 
>> 1. line 72 comment “Five cases:” -> “Four cases:”
>> So you removed case 3 and renumbered, so old references to case 4 -> case 3 
>> ,and old references
>> to case 5 become case 4:
>> 
>> So - line 786, “Case 4” -> “case 3”
>> 
>> thanks,
>> Karen
>> 
>> 
>>> On Feb 12, 2018, at 11:13 AM, harold seigel <harold.sei...@oracle.com 
>>> <mailto:harold.sei...@oracle.com>> wrote:
>>> 
>>> Hi Alan,
>>> 
>>> Thanks for looking at this.
>>> 
>>> Harold
>>> 
>>> On 2/12/2018 2:52 AM, Alan Bateman wrote:
>>>> On 12/02/2018 06:54, David Holmes wrote:
>>>>> Hi Harold,
>>>>> 
>>>>> Adding core-libs-dev so they are aware of the ClassLoader change.
>>>> Thanks, that part is okay and good to see this going away.
>>>> 
>>>> -Alan
>>> 
>> 
> 

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

[Karen Kinnear]

Harold,

Thanks for doing this. 

I think you told me that
1) the version change has made it in
2) you also put 12 as an expiration date
3) you are running the ParallelClassLoading tests with the remaining two flags 
(you’ve already
run them without any flags):
  AllowParallelDefineClass = true and AlwaysLockClassLoader=true

In terms of the guarantee in question
  // For UnsyncloadClass only
 848   // If they got a linkageError, check if a parallel class load 
succeeded.
 849   // If it did, then for bytecode resolution the specification requires
 850   // that we return the same result we did for the other thread, i.e. 
the
 851   // successfully loaded InstanceKlass
 852   // Should not get here for classloaders that support parallelism
 853   // with the new cleaner mechanism, even with AllowParallelDefineClass
 854   // Bootstrap goes through here to allow for an extra guarantee check
 855   if (UnsyncloadClass || (class_loader.is_null())) {
 856 if (k == NULL && HAS_PENDING_EXCEPTION
 857   && 
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
 858   MutexLocker mu(SystemDictionary_lock, THREAD);
 859   InstanceKlass* check = find_class(d_hash, name, dictionary);
 860   if (check != NULL) {
 861 // Klass is already loaded, so just use it
 862 k = check;
 863 CLEAR_PENDING_EXCEPTION;
 864 guarantee((!class_loader.is_null()), "dup definition for 
bootstrap loader?");
 865   }
 866 }
 867   }

  1) I agree you can remove the entire section
  - the guarantee was there for future proofing in case we ever allowed 
parallel class loading of the
same class for the null loader and to make sure I didn’t have any logic 
holes.
  - I would not put an assertion for the first half of the condition - I 
would remove completely
  - the code currently prevents parallel class loading of the same class 
for the null loader at:
resolve_instance_class_or_null see line 785 …
  while (!class_has_been_loaded && old probe && old 
probe->instance_load_in_progress()) {
 //  case x: bootstrap class loader: prevent futile class loading,
 // wait on first requestor
 if (class_loader.is_null()) {
SystemDictionary_lock->wait();

This logic means that there is a registered INSTANCE_LOAD on this placeholder 
entry.


Other minor comments (sorry if you already got these and I missed them in 
earlier emails)
- all in SystemDictionary.cpp

1. line 72 comment “Five cases:” -> “Four cases:”
So you removed case 3 and renumbered, so old references to case 4 -> case 3 
,and old references
to case 5 become case 4:

So - line 786, “Case 4” -> “case 3”

thanks,
Karen


> On Feb 12, 2018, at 11:13 AM, harold seigel  wrote:
> 
> Hi Alan,
> 
> Thanks for looking at this.
> 
> Harold
> 
> On 2/12/2018 2:52 AM, Alan Bateman wrote:
>> On 12/02/2018 06:54, David Holmes wrote:
>>> Hi Harold,
>>> 
>>> Adding core-libs-dev so they are aware of the ClassLoader change.
>> Thanks, that part is okay and good to see this going away.
>> 
>> -Alan
> 

Re: [10] RFR 8186046 Minimal ConstantDynamic support

[Karen Kinnear]

Paul,

Thank you for the explanations. Asking for your help in some test case 
construction at the end here:

>> 3. java/lang/invoke/package-info.java 128-134
>> Error handling could be clearer.
>> My understanding is that if a LinkageError or subclass is thrown, this will 
>> be rethrown
>> for all subsequent attempts. Other errors, e.g. VMError may retry resolution
I was WRONG here - this does match the JVMS. Apologies for the confusion.
>> 
>> 9. 
>> test/jdk/java/lang/invoke/common/test/java/lang/invoke/lib/InstructionHelper.java
>> How would I write an ldc CONSTANT_Dynamic which referred to a bootstrap 
>> method that
>> was not ACC_STATIC?
> 
> https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.23 
> 
> 
> See the note under bootstrap_method_ref. The kind of method handle is 
> constrained because of the arguments that are pushed on the stack before 
> invocation. I believe it’s possible to have a constructor, but the declaring 
> class would need to be a subtype of CallSite in the case of indy, and a 
> subtype of the constant value type in the case of condy.
> 
> 
>> Or was not ACC_PUBLIC?
> 
> That’s dependent on the accessibility between the lookup class the the BSM 
> declaring class.
> 
> 
>> Or was 
>> Or did I read the invoke dynamic method incorrectly?
>> 
> 
> By default, and for convenience, the InstructionHelper assumes the BSM is 
> declared by the lookup class. I recently modified that to support the BSM 
> being declared on another class, to test the minimal set of BSMs for condy 
> (separate issue [1]). Note it’s always possible to use the bytecode API more 
> directly.
> 
> So we can easily add more -ve tests for non-accessible or non-static BSMs.
Thank you - that is what we were trying to do - test BSM declared in another 
class, test in-accessible
BSM and test non static method.

Could you help us figure out how to
1) invoke a constructor? 
2) invoke a virtual method? How do you pass the receiver? 

thanks,
Karen

> 
> Paul.
> 
> [1] 
> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/
>  
> 

Re: [10] RFR 8186046 Minimal ConstantDynamic support

[Karen Kinnear]

Thank you so much for doing this jointly.

Couple of questions/comments:
1. thank you for making UseBootstrapCallInfo diagnostic

2. org/objectweb/asmClassReader.java
why hardcoded 17 instead of ClassWriter.CONDY?

3. java/lang/invoke/package-info.java 128-134
Error handling could be clearer.
My understanding is that if a LinkageError or subclass is thrown, this will be 
rethrown
for all subsequent attempts. Other errors, e.g. VMError may retry resolution

Also: after line 165: rules do not enable the JVM to share call sites.
Is it worth noting anywhere that the Constant_Dynamic resolution is shared?

4. SD::find_java_mirror_for_type
lines 2679+
  ConDy should not be supporting CONSTANT_Class(“;” + FD)
  IIRC that is temporary for MVT but not part of ConDy’s spec, nor part of what
  should be checked in for 18.3
also line 2731 - remove comment about “Foo;”

5. constantTag.hpp
ofBasicType: case T_OBJECT return constantTag(JVM_CONSTANT_String) ?
why not JVM_CONSTANT_CLASS?

6. SD::find_java_mirror_for_type
You have resolve_or_null/fail doing CHECK_(empty) which should
check for a NULL constant_type_klass. This is followed by an explicit if
(constant_type_klass == NULL) — is that needed?

7. reflection.cpp
get_mirror_from_signature
Looks like potential for side effects. Odd to set mirror_oop inside if 
(log_is_enabled)
Is the intent to assert that k->java_mirror() == mirror_oop?
Or is the issue that we have a make drop here and the potential for a safe 
point?
If so, make a handle and extract it on return?

— Or better yet - don’t make any changes to reflection.cpp - not necessary

8. BootstrapMethodInvoker
Could you possibly add a comment that the default today is vmIsPushing and 
IsPullModeBSM is false?
Or find a slightly different naming to make that clearer?

9. 
test/jdk/java/lang/invoke/common/test/java/lang/invoke/lib/InstructionHelper.java
How would I write an ldc CONSTANT_Dynamic which referred to a bootstrap method 
that
was not ACC_STATIC?
Or was not ACC_PUBLIC?
Or was 
Or did I read the invoke dynamic method incorrectly?

thanks,
Karen

> On Oct 26, 2017, at 1:03 PM, Paul Sandoz  wrote:
> 
> Hi,
> 
> Please review the following patch for minimal dynamic constant support:
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/
>  
> 
> 
>  https://bugs.openjdk.java.net/browse/JDK-8186046 
> 
>  https://bugs.openjdk.java.net/browse/JDK-8186209 
> 
> 
> This patch is based on the JDK 10 unified HotSpot repository. Testing so far 
> looks good.
> 
> By minimal i mean just the support in the runtime for a dynamic constant pool 
> entry to be referenced by a LDC instruction or a bootstrap method argument. 
> Much of the work leverages the foundations built by invoke dynamic but is 
> arguably simpler since resolution is less complex.
> 
> A small set of bootstrap methods will be proposed as a follow on issue for 10 
> (these are currently being refined in the amber repository).
> 
> Bootstrap method invocation has not changed (and the rules are the same for 
> dynamic constants and indy). It is planned to enhance this in a further major 
> release to support lazy resolution of bootstrap method arguments.
> 
> The CSR for the VM specification is here:
> 
>  https://bugs.openjdk.java.net/browse/JDK-8189199 
> 
> 
> the j.l.invoke package documentation was also updated but please consider the 
> VM specification as the definitive "source of truth" (we may clean up this 
> area further later on so it becomes more informative, and that may also apply 
> to duplicative text on MethodHandles/VarHandles).
> 
> Any AoT-related work will be deferred to a future release.
> 
> —
> 
> This patch only supports x64 platforms. There is a small set of changes 
> specific to x64 (specifically to support null and primitives constants, as 
> prior to this patch null was used as a sentinel for resolution and certain 
> primitives types would never have been encountered, such as say byte).
> 
> We will need to follow up with the SPARC platform and it is hoped/anticipated 
> that OpenJDK members responsible for other platforms (namely ARM and PPC) 
> will separately provide patches.
> 
> —
> 
> Many of tests rely on an experimental byte code API that supports the 
> generation of byte code with dynamic constants.
> 
> One test uses class file bytes produced from a modified version of asmtools.  
> The modifications have now been pushed but a new version of asmtools need to 
> be rolled into jtreg before the test can operate directly on asmtools 
> information rather than embedding class file bytes directly in the test.
> 
> —
> 
> Paul.

Re: RFR 8169435 : ClassLoader.isParallelCapable is final and conflicting method may get VerifyError

[Karen Kinnear]

+1

Karen

> On Nov 10, 2016, at 5:56 PM, David Holmes  wrote:
> 
> 
> 
> On 11/11/2016 8:46 AM, Mandy Chung wrote:
>> 
>>> On Nov 10, 2016, at 2:28 PM, Peter Levart  wrote:
>>> 
>>> On 11/10/2016 05:59 PM, Alan Bateman wrote:
 
 
 On 10/11/2016 17:42, David M. Lloyd wrote:
> My original suggestion for the method was to make it static, and possibly 
> even caller-sensitive, for just this reason.
 Changing it to be non-final looks reasonable here, the main reason being 
 that it's a no-arg is method and so unlikely that there are custom 
 class loaders that have a method with this name that returns something 
 other than boolean. However the modifier might be a concern and so time 
 will tell if there are custom class loaders that defining a non-public 
 no-arg method with this name.
 
 -Alan
>>> 
>>> It would be nice for this method to be final.
>> 
>> That’d be the ideal case.
>> 
>>> This way it could be relied on to return the "correct" answer regardless of 
>>> the implementation subclass. Who knows, maybe some internal logic might 
>>> need this method in the future and at that time another package-protected 
>>> method would have to be added and exposed via SharedSecrets or similar. If 
>>> "isParallelCapable" is already taken, then what about choosing another name?
>> 
>> This is alternative but it’s hard to predict the probability of a name clash 
>> with existing subclass implementation.
>> 
>>> Since there is already a @CallerSensitive protected static method called 
>>> "registerAsParallelCapable" for subclasses to call from their  
>>> blocks, the query could be called:
>>> 
>>> isRegisteredAsParallelCapable() ?
>>> 
>>> I doubt this name is already taken by any subclass out there…
>> 
>> isRegisteredAsParallelCapable may be okay.
> 
> I'd vote for that and keeping it final.
> 
> Thanks,
> David
> 
>> Mandy
>> 

Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader

[Karen Kinnear]

For the record, I reviewed the jvm changes and they look good.

thanks,
Karen

> On May 4, 2016, at 3:29 PM, Mandy Chung  wrote:
> 
> The default implementation of ObjectInputStream::resolveClass and 
> resolveProxyClass finds the user-defined class loader on the stack and 
> assumes that only system classes are loaded by null loader. As JDK modules 
> are deprivileged, classes on the stack defined by the platform class loader.
> 
> These methods should be updated to prepare if any system class are defined by 
> the platform class loader and its ancestors instead. 
> 
> As for the implementation, I fix JVM_LatestUserDefinedLoader to returns the 
> first non-null class loader on the stack that is not platform class loader.   
> This is so fragile and would be really nice if this  could go away while the 
> past work shows that it’s unlikely - Alan may say more on this.  If this 
> stays, I’d like this to be replaced with StackWalker API and remove such 
> built-in logic in the VM in the future.
> 
> Webrev:
>  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.00/
> 
> Mandy

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Karen Kinnear]

Doug,

I have been thinking about this more from the perspective of the original 
problem
we set out to solve, which was identified in the concurrent hash map usage, at 
the
time in the class loading logic. While the class loading logic has changed, I 
think we
have enough experience with this particular example and have studied
the code constructs sufficiently that there is value in checking in the small 
set of 
JDK changes that target that situation. I also think this gives a sample of
the kind of model in which this approach can be effective. In addition, having 
this small set of
changes provides the ability to test and ensure that the hotspot changes 
continue to
work.

So I would like to recommend that we go ahead and check in the hotspot changes
and the initial minimal set of j.u.c. updates as a way to put the new mechanism
in place so that the people with more domain expertise in the 
java.util.concurrent
libraries can experiment with the mechanism and add incremental improvements.

thanks,
Karen

> On Nov 22, 2015, at 7:04 PM, Doug Lea <d...@cs.oswego.edu> wrote:
> 
> On 11/20/2015 12:40 PM, Karen Kinnear wrote:
>> Totally appreciate the suggestion that the java.util.concurrent modifications
>> be done by folks with more domain expertise.
>> 
>> Would you have us incorporate the initial minimal set of j.u.c. updates or 
>> none
>> at all?
> 
> Sorry that I'm still in foot-drag mode on this.
> Reading David and Fred's exchanges reinforce my thoughts
> that there is no defensible rule or approach to
> use @ReservedStackAccess so as to add as little time and
> space as possible to reduce the occurrence of stuck
> resources as much as possible during StackOverflowError.
> 
> After googling "StackOverflowError java util concurrent" and seeing the
> range of situations that can be encountered, I don't even know
> which kinds of constructions to target.
> And I'm less sure whether using @ReservedStackAccess at all
> is better than doing nothing.
> 
> Maybe there is some decent empirical strategy, but I can't
> tell until hotspot support of @ReservedStackAccess is in place.
> So my vote is still to keep the JDK changes out for now.
> 
> -Doug
> 

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Karen Kinnear]

Frederic,

Looks good.

Many thanks.
Karen

> On Nov 23, 2015, at 12:44 PM, Frederic Parain <frederic.par...@oracle.com> 
> wrote:
> 
> Karen,
> 
> Thank you for your review, my answers are in-lined below.
> 
> New Webrevs (including some fixes suggested by Paul Sandoz):
> 
> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/ 
> <http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/>
> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/ 
> <http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/>
> 
> On 20/11/2015 19:44, Karen Kinnear wrote:
>> Frederic,
>> 
>> Code review for web revs you sent out.
>> Code looks good. I am not as familiar with the compiler code.
>> 
>> I realize you need to check in at least a subset of the java.util.concurrent 
>> changes for
>> the test to work, so maybe I should not have asked Doug about his preference 
>> there.
>> Sorry.
>> 
>> I am impressed you found a way to make a reproducible test!
>> 
>> Comments/questions:
>> 1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”
> 
> Fixed
> 
>> 2. IIRC, due to another bug with windows handling of our guard pages, this
>> is disabled for Windows. Would it be worth putting a comment in the
>> bug , 8067946, that once this is fixed, the reserved stack logic on windows
>> will need testing before enabling?
> 
> More than testing, the implementation would have to be completed on
> Windows. I've added a comment to JDK-8067946.
> 
>> 3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
>> this is in interpreter code, you explicitly return the Java sender
>> of the current frame. I was expecting to see that on Solaris_sparc and 
>> Windows
>> as well? I do see the assertion in caller that you do have a java frame.
> 
> It doesn't make sense to check the current frame (no bytecode has been
> executed yet, so risk of partially executed critical section). I did the
> change but not for all platforms. This is now fixed for Solaris_SPARC
> and Windows too.
> 
>> 4. test line 83 “writtent” -> “written”
> 
> Fixed
> 
>> 5. I like the way you set up the preallocated exception and then set the 
>> message - we may
>> try that for default methods in future.
>> 
>> 6. I had a memory that you had found a bug in safe_for_sender - did you 
>> already check that in?
> 
> I've fixed x86 platforms in JDK-8068655.
> I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
> I had hoped it would have been silently accepted :-)
> 
> 
>> 7. I see the change in trace.xml, and I see an include added to 
>> SharedRuntime.cpp,
>> but I didn’t see where it was used - did I just miss it?
> 
> trace.xml changes define a new event.
> This event is created at sharedRuntime.cpp::3006 and it is used
> in the next 3 lines.
Thanks. I must have mistyped when I searched for it.
> 
> Thanks,
> 
> Fred
> 
> -- 
> Frederic Parain - Oracle
> Grenoble Engineering Center - France
> Phone: +33 4 76 18 81 17
> Email: frederic.par...@oracle.com <mailto:frederic.par...@oracle.com>

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Karen Kinnear]

One other example is an unlock that requires three steps - update state, update 
owner and
unpark successor.

thanks,
Karen

> On Nov 19, 2015, at 9:25 PM, David Holmes  wrote:
> 
> On 20/11/2015 4:10 AM, Doug Lea wrote:
>> On 11/16/2015 08:00 PM, David Holmes wrote:
 I've tried that :-) The amount of code being executed with
 the ReservedStackAccess privilege tends to increase very
 quickly and I was concerned about keeping the size of the
 reserved area small.
>>> 
>>> How much space does each level of calling need? I know that is hard to
>>> answer
>>> but are we talking a few bytes, few kb, many kb?
>>> 
>> 
>> Relatedly, could someone spell out in more detail the
>> worst-case behavior of StackOverflowError? Knowing this
>> might be helpful in some cases in which we might be able to
>> at least preserve minimal sanity without needing to reserve.
> 
> Not sure what you are asking for Doug - worst-case is that an action that is 
> critical to complete does not complete. Best example is lock/unlock which 
> have to both update AQS-state and update owner. If only one is done the lock 
> internal state is inconsistent and the lock ceases to function.
> 
> David
> 
>> -Doug
>> 
>> 
>> 

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Karen Kinnear]

Totally appreciate the suggestion that the java.util.concurrent modifications
be done by folks with more domain expertise.

Would you have us incorporate the initial minimal set of j.u.c. updates or none
at all?


thanks,
Karen

> On Nov 11, 2015, at 8:09 PM, Doug Lea  wrote:
> 
> On 11/10/2015 12:38 PM, frederic parain wrote:
> 
>>> If I put on some extra-strength rose-coloured glasses I think I can
>>> almost read that as "something is better than nothing". ;-)
> 
> Yeah, that's what I meant :-) It is good to at least have the
> @ReservedStackAccess annotation and mechanism in place. Finding
> the best way to use it will take more thought and empirical evaluation.
> 
> So I suggest that the hotspot support part of the webrev be put into place,
> and that we incorporate j.u.c updates using it after further exploration
> and evaluation -- we do have a lot of tests using locks etc, so are in
> a better position to make better decisions about alternative placement
> strategies after trying them.
> 
>>> But note that it is not the finally part that need be at issue.
> 
> Right. But given the existence of @ReservedStackAccess, we might be
> able to rework some code so as to use it more sparingly.
> 
> -Doug
> 
> 

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Karen Kinnear]

Frederic,

Code review for web revs you sent out.
Code looks good. I am not as familiar with the compiler code.

I realize you need to check in at least a subset of the java.util.concurrent 
changes for
the test to work, so maybe I should not have asked Doug about his preference 
there.
Sorry.

I am impressed you found a way to make a reproducible test!

Comments/questions:
1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”

2. IIRC, due to another bug with windows handling of our guard pages, this
is disabled for Windows. Would it be worth putting a comment in the
bug , 8067946, that once this is fixed, the reserved stack logic on windows
will need testing before enabling?

3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
this is in interpreter code, you explicitly return the Java sender
of the current frame. I was expecting to see that on Solaris_sparc and Windows
as well? I do see the assertion in caller that you do have a java frame.

4. test line 83 “writtent” -> “written”

5. I like the way you set up the preallocated exception and then set the 
message - we may
try that for default methods in future.

6. I had a memory that you had found a bug in safe_for_sender - did you already 
check that in?

7. I see the change in trace.xml, and I see an include added to 
SharedRuntime.cpp, 
but I didn’t see where it was used - did I just miss it?

thanks,
Karen

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

[Karen Kinnear]

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?

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(s): 6764713: Enlarge the age field in object headers to allow a higher MaxTenuringThreshold

[Karen Kinnear]

Seconded. For the hash code, talk to Coleen and ask her who did the work in 
core libs recently.

In addition, those bits are fiercely sought after - we have other things we 
would like to do with any available bits and I am
not convinced this is more valuable. We just resisted using one for the jdk9 
frozen arrays although we might want one to mark
immutable objects or value types (yes, today those don't have an identity hash 
but I am not yet convinced that we won't have
a design where we need to distinguish reference objects from value types 
underneath a common object header for jdk10).

So - hmmm. 

thanks,
Karen

On Feb 12, 2015, at 9:54 PM, David Holmes wrote:

 Hi Tom,
 
 If you are potentially messing with the (identity) hash of all Java objects 
 in the 32-bit case then this needs a broader discussion eg on core-libs-dev 
 (cc'd) as this will impact end-user code the most!
 
 The rest seems okay but I'm still mulling over it. :)
 
 Thanks,
 David H.
 
 On 13/02/2015 6:14 AM, Tom Benson wrote:
 Hi,
 I need reviewers and a commit sponsor for changes for bug 6764713, which
 will increase the size of the age field in an object header from 4 bits
 to 5. This will allow a maximum MaxTenuringThreshold of 31, though the
 default will remain at the current value of 15.
 
 This includes the same change to the 32-bit version, which would close
 bug 6719225 as well.  In that case, the hash field in the header is
 affected, losing one bit (25 bits - 24), so I have asked for review
 from hotspot-runtime-dev as well as gc-dev.
 
 Webrev: http://cr.openjdk.java.net/~jprovino/6764713/webrev.00
 JBS bug: https://bugs.openjdk.java.net/browse/JDK-6764713
 Testing:  JPRT and reference server performance tests
 
 Notes:
 Contrary to what earlier notes in the JBS entry said, this does not
 require stronger alignment for the JavaThread structure for when biased
 locking stores that pointer in the header.   The JavaThread* was already
 being aligned 1 power of 2 more strongly than it needed to be, so there
 was an unused bit that could be stolen.
 
 In the 32-bit version, it does require taking one bit from the hash
 field, which goes from 25 to 24 bits.  This is something I'd especially
 like feedback on.  Running reference server performance tests, I saw no
 impact from this change.  We *could* make this change 64-bit-only, and
 leave the age field at 4 bits for the 32-bit version.  If we did so, we
 could also decrease the alignment required for the JavaThread* to 512
 from the current 1024.
 
 The comment changes imply that the bits available for the JavaThread*
 have been reduced by 1, and that the alignment is now stronger, but
 neither is true.  The comments have been corrected to match the
 alignment that was already enforced.
 
 Three tests needed to be corrected to match the new limits.  These check
 the maximum valid values, what value represents NeverTenure, and so on.
 
 Thanks,
 Tom
 

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Karen Kinnear]

Thanks Ioi - looks good.

thanks,
Karen

On Oct 30, 2014, at 1:26 AM, Ioi Lam wrote:

 OK, here's the latest version. I removed the synchronization but kept the 
 resolveClass just for completeness, even if it currently does nothing.
 
class Launcher$AppClassLoader {

public Class? loadClass(String name, boolean resolve)
throws ClassNotFoundException
{
int i = name.lastIndexOf('.');
if (i != -1) {
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPackageAccess(name.substring(0, i));
}
}
 
if (ucp.knownToNotExist(name)) {
// The class of the given name is not found in the parent
// class loader as well as its local URLClassPath.
// Check if this class has already been defined dynamically;
// if so, return the loaded class; otherwise, skip the parent
// delegation and findClass.
Class? c = findLoadedClass(name);
if (c != null) {
if (resolve) {
resolveClass(c);
}
return c;
}
throw new ClassNotFoundException(name);
}
 
return (super.loadClass(name, resolve));
}
 
 Thanks
 
 - Ioi
 
 On 10/29/14, 12:10 PM, Mandy Chung wrote:
 
 On 10/29/2014 7:16 AM, Karen Kinnear wrote:
 Sorry, I was confused about who wrote what.
 
 Sounds like David and I are in agreement that you can remove the 
 synchronization - I believe that would be much cleaner.
 
 I agree that the class loader lock is not really needed in
 the knownToNotExist case as it's checking if the class is
 loaded or not.  Good catch, Karen.
 
 Mandy
 
 And resolveClass does nothing and is final so no worries there.
 
 thanks,
 Karen
 
 On Oct 29, 2014, at 2:37 AM, David Holmes wrote:
 
 On 29/10/2014 4:04 PM, Ioi Lam wrote:
 On 10/28/14, 7:34 PM, David Holmes wrote:
 Hi Karen,
 
 I haven't been tracking the details of this and am unclear on the
 overall caching strategy however ...
 
 On 29/10/2014 8:49 AM, Karen Kinnear wrote:
 Ioi,
 
 Looks good! Thanks to all who have contributed!
 
 A couple of minor comments/questions:
 
 1. jvm.h (hotspot and jdk)
 All three APIs talk about loader_type, but the code uses loader.
 
 2.  Launcher.java
 To the best of my understanding - the call to findLoadedClass does
 not require synchronizing on the class loader lock,
 that is needed to ensure find/define atomicity - so that we do not
 call defineClass twice on the same class - i.e. in
 loadClass - it is needed around the findLoadedClass /
 findClass(defineClass) calls. This call is just a SystemDictionary
 lookup
 and does not require the lock to be held.
 If the class can be defined dynamically - which it appears it can
 (though I'm not sure what that means) - then you can have a race
 between the thread doing the defining and the thread doing the
 findLoadedClass. By doing findLoadedClass with the lock held you
 enforce some serialization of the actions, but there is still a race.
 So the only way the lock could matter is if user code could trigger
 the second thread's lookup of the class after the lock has been taken
 by the thread doing the dynamic definition - whether that is possible
 depends on what this dynamic definition actually is.
 
 I copied the code from ClassLoader.loadClass, which does it it a
 synchronized block:
 
 class ClassLoader {
 protected Class? loadClass(String name, boolean resolve)
 throws ClassNotFoundException
 {
 synchronized (getClassLoadingLock(name)) {
 // First, check if the class has already been loaded
 Class? c = findLoadedClass(name);
 if (c == null) {
 long t0 = System.nanoTime();
 try {
 if (parent != null) {
 c = parent.loadClass(name, false);
 } else {
 c = findBootstrapClassOrNull(name);
 }
 } catch (ClassNotFoundException e) {
 // ClassNotFoundException thrown if class not found
 // from the non-null parent class loader
 }
 
 if (c == null) {
 // If still not found, then invoke findClass in order
 // to find the class.
 long t1 = System.nanoTime();
 c = findClass(name);
 
 // this is the defining class loader; record the stats
 sun.misc.PerfCounter.getParentDelegationTime().addTime(t1 - t0);
 sun.misc.PerfCounter.getFindClassTime().addElapsedTimeFrom(t1);
 sun.misc.PerfCounter.getFindClasses().increment();
 }
 }
 if (resolve) {
 resolveClass(c

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Karen Kinnear]

David,

On Oct 29, 2014, at 2:04 AM, Ioi Lam wrote:

 
 On 10/28/14, 7:34 PM, David Holmes wrote:
 Hi Karen, 
 
 I haven't been tracking the details of this and am unclear on the overall 
 caching strategy however ... 
 
 On 29/10/2014 8:49 AM, Karen Kinnear wrote: 
 Ioi, 
 
 Looks good! Thanks to all who have contributed! 
 
 A couple of minor comments/questions: 
 
 1. jvm.h (hotspot and jdk) 
 All three APIs talk about loader_type, but the code uses loader. 
 
 2.  Launcher.java 
 To the best of my understanding - the call to findLoadedClass does not 
 require synchronizing on the class loader lock, 
 that is needed to ensure find/define atomicity - so that we do not call 
 defineClass twice on the same class - i.e. in 
 loadClass - it is needed around the findLoadedClass / 
 findClass(defineClass) calls. This call is just a SystemDictionary lookup 
 and does not require the lock to be held. 
 
 If the class can be defined dynamically - which it appears it can (though 
 I'm not sure what that means) - then you can have a race between the thread 
 doing the defining and the thread doing the findLoadedClass. By doing 
 findLoadedClass with the lock held you enforce some serialization of the 
 actions, but there is still a race. So the only way the lock could matter is 
 if user code could trigger the second thread's lookup of the class after the 
 lock has been taken by the thread doing the dynamic definition - whether 
 that is possible depends on what this dynamic definition actually is. 
findLoadedClass is always a snapshot, i.e. I agree with you that with or 
without a lock it is possible that another thread will define the class you
are looking for after the findLoadedClass returns failure.

Defined dynamically here is in contrast to being found in the archive - and 
uses the normal defineClass code paths.

Going back to double-check in our parallel class loading changes to class 
loader synchronization:
http://openjdk.java.net/groups/core-libs/ClassLoaderProposal.html

This does match my memory - which is that the synchronization is to prevent 
duplicate defineClass calls, and we deliberately
made the following changes to java.lang.ClassLoader:

protected loadClass(String, boolean) changes:

Remove synchronized keyword

If this is not a parallel capable class loader, synchronize on this for 
backward compatibility

else synchronize on a class-name-based-lock

The synchronization in protected loadClass(String, boolean) ensures that 
defineClass(...) will not be called multiple times in parallel for the same 
class name/class loader pair.



and recommendations for other class loaders:
4.b. Class loaders that invoke registerAsParallelCapable(), that override 
protected loadClass(String, boolean) or public loadClass(String)

We recommend that you override findClass(String) instead, if at all possible

To ensure that the protected defineClass(...) method is only called once for 
the same class name, you need to implement a finer-grained locking scheme. One 
option would be to adopt the class name based locking mechanism from 
java.lang.ClassLoader's protected loadClass(String, boolean) method.


Note also that resolveClass does nothing (it throws an exception if called with 
a null value).

So - my conclusion is we do not need the lock.

David - I believe you might be improving the parallel class loading logic going 
forward, exploring the possibility of
allowing parallel defineClass (with spec modifications). I don't think any 
synchronization is going to help us with
that cleanup or with performance. And I think it will confuse people into 
worrying that the synchronization is needed here.
If you want to leave it in anyway, please add some sort of comment that the 
synchronized is paranoia rather than
having a known reason to use it - or that Karen doesn't believe it is needed 
:-) - or something - to not prevent future
improvements here.

thanks,
Karen

 
 I copied the code from ClassLoader.loadClass, which does it it a synchronized 
 block:
 
 class ClassLoader {
 protected Class? loadClass(String name, boolean resolve)
 throws ClassNotFoundException
 {
 synchronized (getClassLoadingLock(name)) {
 // First, check if the class has already been loaded
 Class? c = findLoadedClass(name);
 if (c == null) {
 long t0 = System.nanoTime();
 try {
 if (parent != null) {
 c = parent.loadClass(name, false);
 } else {
 c = findBootstrapClassOrNull(name);
 }
 } catch (ClassNotFoundException e) {
 // ClassNotFoundException thrown if class not found
 // from the non-null parent class loader
 }
 
 if (c == null) {
 // If still not found, then invoke findClass in order
 // to find the class

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Karen Kinnear]

Sorry, I was confused about who wrote what.

Sounds like David and I are in agreement that you can remove the 
synchronization - I believe that
would be much cleaner.
And resolveClass does nothing and is final so no worries there.

thanks,
Karen

On Oct 29, 2014, at 2:37 AM, David Holmes wrote:

 On 29/10/2014 4:04 PM, Ioi Lam wrote:
 
 On 10/28/14, 7:34 PM, David Holmes wrote:
 Hi Karen,
 
 I haven't been tracking the details of this and am unclear on the
 overall caching strategy however ...
 
 On 29/10/2014 8:49 AM, Karen Kinnear wrote:
 Ioi,
 
 Looks good! Thanks to all who have contributed!
 
 A couple of minor comments/questions:
 
 1. jvm.h (hotspot and jdk)
 All three APIs talk about loader_type, but the code uses loader.
 
 2.  Launcher.java
 To the best of my understanding - the call to findLoadedClass does
 not require synchronizing on the class loader lock,
 that is needed to ensure find/define atomicity - so that we do not
 call defineClass twice on the same class - i.e. in
 loadClass - it is needed around the findLoadedClass /
 findClass(defineClass) calls. This call is just a SystemDictionary
 lookup
 and does not require the lock to be held.
 
 If the class can be defined dynamically - which it appears it can
 (though I'm not sure what that means) - then you can have a race
 between the thread doing the defining and the thread doing the
 findLoadedClass. By doing findLoadedClass with the lock held you
 enforce some serialization of the actions, but there is still a race.
 So the only way the lock could matter is if user code could trigger
 the second thread's lookup of the class after the lock has been taken
 by the thread doing the dynamic definition - whether that is possible
 depends on what this dynamic definition actually is.
 
 I copied the code from ClassLoader.loadClass, which does it it a
 synchronized block:
 
 class ClassLoader {
 protected Class? loadClass(String name, boolean resolve)
 throws ClassNotFoundException
 {
 synchronized (getClassLoadingLock(name)) {
 // First, check if the class has already been loaded
 Class? c = findLoadedClass(name);
 if (c == null) {
 long t0 = System.nanoTime();
 try {
 if (parent != null) {
 c = parent.loadClass(name, false);
 } else {
 c = findBootstrapClassOrNull(name);
 }
 } catch (ClassNotFoundException e) {
 // ClassNotFoundException thrown if class not found
 // from the non-null parent class loader
 }
 
 if (c == null) {
 // If still not found, then invoke findClass in order
 // to find the class.
 long t1 = System.nanoTime();
 c = findClass(name);
 
 // this is the defining class loader; record the stats
 sun.misc.PerfCounter.getParentDelegationTime().addTime(t1 - t0);
 sun.misc.PerfCounter.getFindClassTime().addElapsedTimeFrom(t1);
 sun.misc.PerfCounter.getFindClasses().increment();
 }
 }
 if (resolve) {
 resolveClass(c);
 }
 return c;
 }
 }
 
 So I guess it should look like this in Launcher$AppClassLoader, just to
 ensure the same things are done (regardless of whether it's necessary or
 not)?
 
 In ClassLoader.loadClass it is providing atomicity across a number of actions 
 in the worst-case:
 - checking for already loaded; if not then
 - try to load through parent; if not then
 - findClass (which will do defineClass)
 
 You don't have those atomicity constraints because you are only doing one 
 thing - checking to see if the class is loaded.
 
 Your locking is probably harmless but those are famous last words when it 
 comes to classloading. :)
 
 
 Does resolveClass need to be done inside the synchronized block?
 
 Depends on whether it depends on the classloader locking to prevent 
 concurrent resolve attempts.
 
 David
 -
 
 class Launcher$AppClassLoader {
 public Class? loadClass(String name, boolean resolve)
 throws ClassNotFoundException
 {
 int i = name.lastIndexOf('.');
 if (i != -1) {
 SecurityManager sm = System.getSecurityManager();
 if (sm != null) {
 sm.checkPackageAccess(name.substring(0, i));
 }
 }
 
 if (ucp.knownToNotExist(name)) {
 // The class of the given name is not found in the parent
 // class loader as well as its local URLClassPath.
 // Check if this class has already been defined
 dynamically;
 // if so, return the loaded class; otherwise, skip the
 parent
 // delegation and findClass.
 from here

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Karen Kinnear]

Ioi,

Looks good! Thanks to all who have contributed!

A couple of minor comments/questions:

1. jvm.h (hotspot and jdk)
All three APIs talk about loader_type, but the code uses loader.

2.  Launcher.java
To the best of my understanding - the call to findLoadedClass does not require 
synchronizing on the class loader lock,
that is needed to ensure find/define atomicity - so that we do not call 
defineClass twice on the same class - i.e. in
loadClass - it is needed around the findLoadedClass / findClass(defineClass) 
calls. This call is just a SystemDictionary lookup
and does not require the lock to be held.

David H and Mandy - does that make sense to you both?

thanks,
Karen

On Oct 28, 2014, at 12:38 AM, Ioi Lam wrote:

 
 On 10/27/14, 7:04 PM, Mandy Chung wrote:
 
 On 10/27/2014 3:32 PM, Ioi Lam wrote:
 Hi David, I have update the latest webrev at:
 
 http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/
 
 
 The update looks good.  Thanks.
 
 This version also contains the JDK test case that Mandy requested:
 
 http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/jdk/test/sun/misc/URLClassPath/EnableLookupCache.java.html
  
 
 
 What I request to add is a test setting the system property 
 (-Dsun.cds.enableSharedLookupCache=true) and continue to load class A and B. 
  Removing line 44-58 should do it and also no need to set -Dfoo.foo.bar.
 
 Do you mean change the test to call 
 System.setProperty(sun.cds.enableSharedLookupCache, true)?
 
 But we know that the property is checked only once, before any app classes 
 are loaded. So calling System.setProperty in an application class won't test 
 anything.
 It'd be good if you run this test and turn on the debug traces to make sure 
 that the application class loader and ext class loader will start up with 
 the lookup cache enabled and make up call to the VM.  As it doesn't have the 
 app cds archive, it will invalidate the cache right away and continue the 
 class lookup with null cache array.
 In the latest code, if CDS is not available, lookupCacheEnabled will be set 
 to false inside the static initializer of URLClassPath:
 
private static volatile boolean lookupCacheEnabled
= 
 true.equals(VM.getSavedProperty(sun.cds.enableSharedLookupCache));
 
 later, when the boot/ext/app loaders call into here:
 
synchronized void initLookupCache(ClassLoader loader) {
if ((lookupCacheURLs = getLookupCacheURLs(loader)) != null) {
lookupCacheLoader = loader;
} else {
// This JVM instance does not support lookup cache.
disableAllLookupCaches();
}
}
 
 their lookupCacheURLs[] fields will all be set to null. As a result, 
 getLookupCacheForClassLoader and knownToNotExist0 will never be called.
 
 I can add a DEBUG_LOOKUP_CACHE trace inside disableAllLookupCaches to print 
 lookup cache disabled, and check for that in the test. Is this OK?
 
 Thanks
 - Ioi
 
 
 

Re: Loading classes with many methods is very expensive

[Karen Kinnear]

Peter,

Which hotspot algorithm is the one you identified as slow? Would that be
for loading the class or for Class.getMethods?

thanks,
Karen

On Oct 23, 2014, at 9:37 AM, Peter Levart wrote:

 On 10/23/2014 01:57 AM, Stanimir Simeonoff wrote:
 Class.java can easily be improved by adding an auxiliary  HasMapString,
 Integer/int[] indexing the position in the array by name and then checking
 only few overloaded signatures in case of a match.
 The auxiliary map can be deployed only past some threshold of methods, so
 it should not affect the common case.
 
 Alternatively sorting the array and using binary search is quite trivial to
 implement as well.
 
 Java Class.getMethods() implementation is complicated by the fact that, 
 although not specified, the order of methods in returned array is important. 
 Once it changed, if I remember correctly, and broke many programs, so it had 
 to be restored...
 
 Peter
 
 
 Btw, really nice benchmark.
 
 Stanimir
 
 
 On Thu, Oct 23, 2014 at 1:53 AM, Martin Buchholz marti...@google.com
 wrote:
 
 Here at Google we have both kinds of scalability problems - loading classes
 from a classpath with 10,000 jars, and loading a single class file packed
 with the maximal number of methods.  This message is about the latter.
 
 If you have a class with ~64k methods with a superclass that also has ~64k
 methods, class loading that single class will cost you ~30sec and calling
 Class.getMethods another ~10sec.  Both are unacceptably slow. I think both
 are due to O(N^2) algorithms, the first in hotspot, and the second in
 Class.java.
 
 I have the start of a fix for Class.java, but it makes the common case
 slower.  A really good fix is harder to find.  In general, I think
 Class.java could benefit from some performance-oriented rework.  Is anyone
 else working on class loading performance, especially in hotspot?
 
 Here's the benchmark (that could perhaps be integrated into openjdk even
 without a fix)
 
 
 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class.getMethods-benchmark/test/java/lang/Class/getMethods/ManyMethodsBenchmark.java.html
 
 Base class load time: 186.44 ms
 getDeclaredMethods: Methods: 65521, Total time: 43.27 ms, Time per method:
 0.0007 ms
 getMethods: Methods: 65530, Total time: 60.82 ms, Time per method:
 0.0009 ms
 Derived class load time: 33440.13 ms
 getDeclaredMethods: Methods: 65521, Total time: 39.71 ms, Time per method:
 0.0006 ms
 getMethods: Methods: 65530, Total time: 11582.54 ms, Time per
 method: 0.1768 ms
 
 

Re: RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, final round

[Karen Kinnear]

Looks good. Ship it!

thank you so much Ioi, Jiangli, Calvin, Yumin and David for all the hard work!
Karen

On Aug 13, 2014, at 2:25 AM, Ioi Lam wrote:

 Hi,
 
 Thank you all for the reviews! I have prepared a final version that has 
 incorporated all the comments.
 
 http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-vfinal/
 
 - Ioi
 
 On 8/9/14, 1:02 AM, Ioi Lam wrote:
 Hi,
 
 Thanks a lot to everyone for the very useful comments. I have updated the 
 webrev
 
 Just the delta from the previous round of review:
 
 http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3_delta_from_v2/
 
 All the changes:
 
 http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3/
 
 Thanks
 - Ioi
 
 On 7/28/14, 4:09 PM, Ioi Lam wrote:
 Hi Folks,
 
 Please review the following clean up and refactoring of the CDS code, for 
 JDK9
 
 http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v2/
 https://bugs.openjdk.java.net/browse/JDK-8046070
 
 Summary of fix:
 
Clean up and refactor the Class Data Sharing (CDS) code, including:
 
+ Improve archive integrity checking
+ Support bytecode verification during archive dumping time
+ Allow the user to configure the CDS class list and archive file.
+ Allow future extension of the CDS class loading mechanism.
 
 Tests:
 
JPRT
UTE (vm.runtime.testlist, vm.quick.testlist, 
 vm.parallel_class_loading.testlist)
Various adhoc SQE tests on Aurora
JCK
 
 Thanks
 - Ioi
 
 

Re: RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, round #2

[Karen Kinnear]

Ioi,

Changes look really good. Just minor comments:

1. jdk: SecureClassLoader.java: getProtectionDomain
please add a comment that this assumes no signers and combine the first and 
third lines

2. classFileParser.cpp
Would it make sense to move the assertion from skip_over_field_name for 
DumpSharedSpaces
to JavaCalls::call* ?

3. classLoader.cpp
  VM internal error. Must not load .class file during dump time
   - I think what you are saying is that class data sharing only supports 
loading .class files from .jar files - maybe
   modify the error message

4. classLoader.cpp
  line 258: could you fix mmaped - mmapped

5. classLoader.cpp
  lines 468, 519: I think this changes the TraceClassLoading behavior? I think 
you want
   if (TraceClassLoading || TraceClassPaths  Verbose) to print_meta_index (or 
maybe TraceClassPaths || (TraceClassLoading  Verbose)
   same with line 549- PrintSharedArchiveAndExit - really you mean same as 
above?

6. classLoader.cpp 
   check_shared_classpath
   - what does CDS before these changes do if you have an empty path in the 
archived classpath or a non-empty directory?
   - what happens if the archive is created (DumpSharedSpaces) without an empty 
path or with a non-empty directory
 but used with an empty path element or a non-empty directory?
line 587 calls check_shared_classpath on if DumpSharedSpaces?

7. classLoader.cpp
   line 1085: you have a ResourceMark when you created class_name, so you don't 
need another one
   just a note: the class_name is legally allowed to be null - if you had 
succeeded in parseClassFile you would use
   parsed_name for any printing of the result (your code is fine, just wanted 
you to know)

8. dictionary.cpp
 line 225 - I presume this is for a specific compiler? If you know what it is, 
it would help to record it here
in case in future we could move this

9. systemDictionary.cpp
   comment lines 1221-1224

10. sharedPathsMiscInfo.hpp
   alst- also (spelling)

11. SharedPathsMiscInfo
   Why do you put this in metaspace if it is deallocated after initialization? 
I would have expected a CHeapObj not in metaspace?
   Or if metaspace makes sense, maybe ask Coleen how other metaspace 
information that is freed gets recorded (did we need the new metaspace object 
type Deallocated?)
 is this related to metadataFactory.hpp lines 82-87?

12. Just checking for documentation for use
   If you created a CDS archive (version 1), and then try to use it with the 
new code - will the archive continue to work as it
   did before? Or do you have to recreated it as a version 2 archive? I presume 
we need to document this change. Is it the
   case that an archive is expected to match a given jdk (or hotspot) version? 
So this is not a surprise to customers? Or is
   this behavior new with 8u40?

13. filemap.hpp
  FIXME comment 129 - what is the current state of this? Is this a planned 
future change?

14. metaspaceShared.cpp: 
   line 861: druing - during
   not new: lines 779/780: Comment says Rewrite and unlink classes, prints 
Rewriting and linking classes 
   Maybe the comment could say: Rewrite and link missing classes, then unlink 
all classes as part of dump

   Your comment says super interfaces may have been missed from the classlist(s)
 - I believe that loading any class requires preloading any supertypes - 
superclasses and superinterfaces - so I would
   not expect those to be missing at this time - or is that just in the 
case of failed verification?
 
15. metaspaceShared.cpp
line 682: could you possibly change the comment from repeat to iterate?

16. metaspaceShared.cpp
_check_one_shared_class: at this point you are checking supertypes for 
failed verification
1) have you ever seen a class that passed verification but one of its 
supertypes failed? I don't think that should be possible?
I'm not sure what this loop is for? Just for the 
IgnoreUnverifiableClassesDuringDump?

I have to go to a meeting, so more later.

thanks,
Karen

On Jul 28, 2014, at 7:09 PM, Ioi Lam wrote:

 Hi Folks,
 
 Please review the following clean up and refactoring of the CDS code, for JDK9
 
http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v2/
 https://bugs.openjdk.java.net/browse/JDK-8046070
 
 Summary of fix:
 
Clean up and refactor the Class Data Sharing (CDS) code, including:
 
+ Improve archive integrity checking
+ Support bytecode verification during archive dumping time
+ Allow the user to configure the CDS class list and archive file.
+ Allow future extension of the CDS class loading mechanism.
 
 Tests:
 
JPRT
UTE (vm.runtime.testlist, vm.quick.testlist, 
 vm.parallel_class_loading.testlist)
Various adhoc SQE tests on Aurora
JCK
 
 Thanks
 - Ioi

Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method

[Karen Kinnear]

David,

Thank you so much for finding a way to do this. We do think this is important 
to get in for 8.
And thank you for a way to check the hotspot changes in without waiting for the 
jdk changes.

Code looks good.

Couple of minor comments:
1. universe.cpp - when the hotspot change gets in and the jdk change isn't in 
yet, are
we going to see the tty-print_cr message?
2. style: Method* rather than Method  space *
3. Head's up that ASM is being updated for default method support
  - visitMethoInsn will take an additional argument at the end for 
isinterface to determine
if it needs a methodref or an interfacemethodref in the constant pool. I think 
that is in b117 (?)
so we don't have it yet - so this will need modifying later - don't let that 
stop you from pushing the
fix.

See Lois's test webrev 
http://javaweb.us.oracle.com/~lfoltan/webrev/defmeth_asm_interfacemethodef_2/src/vm/runtime/defmeth/shared/ClassFileGenerator.java.sdiff.html

thanks,
Karen

On Nov 22, 2013, at 2:07 PM, David Chase wrote:

 Updated request.
 
 This is for a bug that it deferred by compilers,
 but runtime really wants it fixed because they
 are working in the same area and don't like it at all.
 In particular, they want it committed to hotspot-rt ASAP
 (don't want to wait for the multiweek turnaround)
 and thus the diffs and testing are against hotspot-rt.
 
 There are jdk and hotspot webrevs; the hotspot webrev
 is designed to allow it to run (with old behavior) in the
 absence of the jdk change.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8016839
 Related test bug: https://bugs.openjdk.java.net/browse/JDK-8027733 
 (confidential, sorry)
 
 Webrev(s):
 http://cr.openjdk.java.net/~drchase/8016839/webrev-hotspot.01/
 http://cr.openjdk.java.net/~drchase/8016839/webrev-jdk.01/
 
 Testing:
 ute vm.quick.testlist A/B, no (new) failures
 jtreg compiler runtime sanity, no (new) failures
 defmeth, no new failures, fewer old failures
 
 New test, specifically tested on Sparc, Intel, embedded
 -Xint, -Xcomp, plain, and with jdb on embedded, and
 Netbeans on x86.  (this was specifically to address the main
 uncertainty, which was calling a zero-arg exception-throwing
 method instead of the expected perhaps-multi-arg inaccessible
 method; it is possible to devise calling conventions that will
 make that not work, but we appear not to have done this.  The test
 includes 0- and 11-arg instances of the not-legally-accessible
 methods).
 
 Problem: 
 Various patterns of inheritance and faulty overriding are supposed to
 throw IllegalAccessError (IAE). At the time the bug was filed, they
 instead threw NullPointerException (NPE) but after a related bug was
 fixed the wrong exception changed to AbstractMethodError (AME).
 
 The root cause of the error is that (1) by convention, if an itable
 entry is null, AME is thrown, and (2) in the case of inaccessible
 methods overriding other methods that implement interface methods, a
 null was entered into the itable (leading to throws of AME on code paths
 that use the itable).
 
 This is a long-standing problem dating back to at least JDK 6, but made
 much easier to observe by the introduction of method handles.  Before
 methodhandles this bug was more difficult to observe because resolution
 of invokeinterface would aggressively evaluate errors and leave the
 invoke unresolved if there was an error; thus repeated testing with
 error receivers would always throw the proper error.  If, however, a
 good receiver is used for the invokeinterface, then resolution succeeds,
 and subsequent invocations with bad receivers throws the wrong error
 (AME instead of NPE) because the resolved code goes directly to the
 itable and sees the null method.
 
 With Methodhandles, resolution and invocation are separate, and it
 always throws the wrong error.
 
 Fix:
 
 I considered (and discussed with John Rose and Coleen Phillimore and
 Karen Kinnear) use of a second null value, where instead of using 0x0
 for the missing method, I would use 0x1, and then special case the null
 checks (in triplicate, across all platforms) to instead unsigned compare
 against a small integer and also special-case the trap handler for this
 value.
 
 This seemed like a big change, potentially confusing to C programmers,
 hacky, and somewhat hateful.
 
 I next considered creating a bridge to nowhere stub that would always
 throw IAE, and fill that Method * in instead of null.  Unfortunately,
 the need for this stub is not discovered until after constant pool
 rewriting has occurred, and adding new methods and CPC entries after
 the rewriting looked very hairy.
 
 The last choice was to define a helper method in sun.misc.Unsafe that
 would always throw IllegalAccessError when called, and use that Method *
 in the should-throw-IAE case.  The risk here is that the helper method
 is zero-arg, where the method about to be called (and whose parameters
 had been pushed on the stack) could have a different set of arguments,
 and perhaps

hg: jdk8/tl/hotspot: 8026893: Push 8026365 to TL early and add test

[karen . kinnear]

Changeset: e39b138b2518
Author:acorn
Date:  2013-10-19 18:32 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/hotspot/rev/e39b138b2518

8026893: Push 8026365 to TL early and add test
Reviewed-by: dcubed, kamg

! src/share/vm/classfile/verifier.cpp
! test/TEST.groups
+ test/runtime/8026365/InvokeSpecialAnonTest.java

Re: RFR: Lambda 8026213: Reflection support for private methods in interfaces

[Karen Kinnear]

Thank you David - that will be helpful.
And thanks for the re-review.

thanks,
Karen

On Oct 11, 2013, at 12:56 AM, David Holmes wrote:

 Karen,
 
 You can set the system property sun.reflect.inflationThreshold=N to change 
 the magic value from 15. That makes testing easier.
 
 David
 
 On 11/10/2013 12:32 AM, Karen Kinnear wrote:
 Joel,
 
 Thank you for the review.
 
 Actually, that is a very good question.
 We will be adding the test case for this one to our vm side 
 defmeth.PrivateMethodsTest, because
 you have to create a classfile since private methods in interfaces are not 
 supported by javac, they
 are for internal use for Lambda support. So it was decided to not try to add 
 them to the
 java reflection tests.
 
 Good question - I brought this up once with Tristan and Amy who were writing 
 reflection tests for lambda.
 I will ask again about reflection coverage for invoking a method more than 
 15 times.
 
 thanks,
 Karen
 
 On Oct 10, 2013, at 9:02 AM, Joel Borggren-Franck wrote:
 
 Hi Karen,
 
 I agree with the others, the code looks good though I like Paul's
 suggestion.
 
 Silly question perhaps, do you know if we have good test coverage on
 actually reflectively invoking a Method more than 15 times?
 
 cheers
 /Joel
 
 On 2013-10-09, Karen Kinnear wrote:
 
 Please review:
 
 webrev: http://cr.openjdk.java.net/~acorn/8026213/webrev/
 bug: https://bugs.openjdk.java.net/browse/JDK-8026213
 
 Summary:
 Reflection generates code dynamically to speed up reflection processing 
 after startup. The first
 15 runs of a reflection call  use the vm code path, after that we use the 
 generated code path, which
 needs to use invokespecial on private methods in interfaces.
 
 Tested:
 Test attached to the bug
 
 Also - all the 8011311 private method testing was run with this in the 
 build:
 Robert Field's TypeTest
 8025475 test
 defmeth privatemethodstest with reflection
 John Rose's intfbug
 jtreg: java.util, java.lang
 jck vm, lang
 
 thanks,
 Karen
 
 
 

Re: RFR: Lambda 8026213: Reflection support for private methods in interfaces

[Karen Kinnear]

Paul,

Thank you so much for the review and suggestion. I will rewrite this this way 
and resend.

thanks!
Karen

On Oct 10, 2013, at 5:13 AM, Paul Sandoz wrote:

 
 On Oct 10, 2013, at 12:13 AM, Karen Kinnear karen.kinn...@oracle.com wrote:
 
 
 Please review:
 
 webrev: http://cr.openjdk.java.net/~acorn/8026213/webrev/
 bug: https://bugs.openjdk.java.net/browse/JDK-8026213
 
 Summary:
 Reflection generates code dynamically to speed up reflection processing 
 after startup. The first
 15 runs of a reflection call  use the vm code path, after that we use the 
 generated code path, which
 needs to use invokespecial on private methods in interfaces.
 
 
 You don't need to pass modifiers as a parameter to emitInvoke since it is set 
 as a field on AccessorGenerator and used, e.g. see isStatic().
 
 So perhaps add the following method to AccessorGenerator:
 
protected boolean isPrivate() {
return Modifier.isPrivate(modifiers);
}
 
 and do:
 
 642 if (isInterface()) {
 
 643 if (isPrivate()) {
 644   cb.opc_invokespecial(targetMethodRef, count, 0);
 645 } else {
 
 646   cb.opc_invokeinterface(targetMethodRef,
 647  count,
 648  count,
 649  
 typeSizeInStackSlots(returnType));
 
 650 }
 
 651 } else {
 652 cb.opc_invokevirtual(targetMethodRef,
 653  count,
 654  
 typeSizeInStackSlots(returnType));
 655 }
 656 }
 657 }
 
 ?
 
 Paul.
 
 Tested:
 Test attached to the bug
 
 Also - all the 8011311 private method testing was run with this in the build:
 Robert Field's TypeTest
 8025475 test
 defmeth privatemethodstest with reflection
 John Rose's intfbug
 jtreg: java.util, java.lang
 jck vm, lang
 
 thanks,
 Karen
 
 
 

Re: RFR: Lambda 8026213: Reflection support for private methods in interfaces

[Karen Kinnear]

Thank you Remi.

I'm actually going to leave the assertion. We are frequently asked to expand 
limits and
catching this if that time ever happens would make our lives much easier.

thanks,
Karen

On Oct 10, 2013, at 5:21 AM, Remi Forax wrote:

 On 10/10/2013 12:13 AM, Karen Kinnear wrote:
 Please review:
 
 webrev: http://cr.openjdk.java.net/~acorn/8026213/webrev/
 bug: https://bugs.openjdk.java.net/browse/JDK-8026213
 
 Summary:
 Reflection generates code dynamically to speed up reflection processing 
 after startup. The first
 15 runs of a reflection call  use the vm code path, after that we use the 
 generated code path, which
 needs to use invokespecial on private methods in interfaces.
 
 Tested:
 Test attached to the bug
 
 Also - all the 8011311 private method testing was run with this in the build:
 Robert Field's TypeTest
 8025475 test
 defmeth privatemethodstest with reflection
 John Rose's intfbug
 jtreg: java.util, java.lang
 jck vm, lang
 
 thanks,
 Karen
 
 
 
 Hi Karen,
 the code is Ok for me.
 
 By the way, at some point the comment at the top of emitInvoke() and the 
 corresponding test
 should be removed because you can not have more than 255/256 parameters for a 
 method.
 
 cheers,
 Rémi
 

Re: RFR: Lambda 8026213: Reflection support for private methods in interfaces

[Karen Kinnear]

David,

Thanks for the information. 
The VM side of this is 8011311 which pushed on 10/8 to jdk8 master to be part 
of JDK8b111.

thanks,
Karen

On Oct 10, 2013, at 5:06 AM, David Holmes wrote:

 Hi Karen,
 
 On 10/10/2013 8:13 AM, Karen Kinnear wrote:
 
 Please review:
 
 webrev: http://cr.openjdk.java.net/~acorn/8026213/webrev/
 bug: https://bugs.openjdk.java.net/browse/JDK-8026213
 
 Summary:
 Reflection generates code dynamically to speed up reflection processing 
 after startup. The first
 15 runs of a reflection call  use the vm code path, after that we use the 
 generated code path, which
 needs to use invokespecial on private methods in interfaces.
 
 Minor nit:
 
  30 import java.lang.reflect.*;
 
 The approved style these days is to list the exact imports needed rather than 
 rely on import-on-demand.
 
 Otherwise change seems fine. Is the VM side of this already in place or does 
 this need to sync with a VM change?
 
 Thanks,
 David
 
 
 Tested:
 Test attached to the bug
 
 Also - all the 8011311 private method testing was run with this in the build:
 Robert Field's TypeTest
 8025475 test
 defmeth privatemethodstest with reflection
 John Rose's intfbug
 jtreg: java.util, java.lang
 jck vm, lang
 
 thanks,
 Karen
 
 

Re: RFR: Lambda 8026213: Reflection support for private methods in interfaces

[Karen Kinnear]

Joel,

Thank you for the review.

Actually, that is a very good question.
We will be adding the test case for this one to our vm side 
defmeth.PrivateMethodsTest, because
you have to create a classfile since private methods in interfaces are not 
supported by javac, they
are for internal use for Lambda support. So it was decided to not try to add 
them to the
java reflection tests.

Good question - I brought this up once with Tristan and Amy who were writing 
reflection tests for lambda.
I will ask again about reflection coverage for invoking a method more than 15 
times.

thanks,
Karen

On Oct 10, 2013, at 9:02 AM, Joel Borggren-Franck wrote:

 Hi Karen,
 
 I agree with the others, the code looks good though I like Paul's
 suggestion.
 
 Silly question perhaps, do you know if we have good test coverage on
 actually reflectively invoking a Method more than 15 times?
 
 cheers
 /Joel
 
 On 2013-10-09, Karen Kinnear wrote:
 
 Please review:
 
 webrev: http://cr.openjdk.java.net/~acorn/8026213/webrev/
 bug: https://bugs.openjdk.java.net/browse/JDK-8026213
 
 Summary:
 Reflection generates code dynamically to speed up reflection processing 
 after startup. The first
 15 runs of a reflection call  use the vm code path, after that we use the 
 generated code path, which
 needs to use invokespecial on private methods in interfaces.
 
 Tested:
 Test attached to the bug
 
 Also - all the 8011311 private method testing was run with this in the build:
 Robert Field's TypeTest
 8025475 test
 defmeth privatemethodstest with reflection
 John Rose's intfbug
 jtreg: java.util, java.lang
 jck vm, lang
 
 thanks,
 Karen
 
 

Re: RFR: Lambda 8026213: Reflection support for private methods in interfaces

[Karen Kinnear]

Updated webrev:
 webrev: http://cr.openjdk.java.net/~acorn/8026213.2/webrev/
 bug: https://bugs.openjdk.java.net/browse/JDK-8026213


Testing:
Specific test for private methods (attached to bug)
jtreg jdk java.lang, sun.reflect

thanks,
Karen

p.s. this should look very much like Paul's suggestion - thank you again

On Oct 10, 2013, at 9:47 AM, Karen Kinnear wrote:

 Paul,
 
 Thank you so much for the review and suggestion. I will rewrite this this way 
 and resend.
 
 thanks!
 Karen
 
 On Oct 10, 2013, at 5:13 AM, Paul Sandoz wrote:
 
 
 On Oct 10, 2013, at 12:13 AM, Karen Kinnear karen.kinn...@oracle.com wrote:
 
 
 Please review:
 
 webrev: http://cr.openjdk.java.net/~acorn/8026213/webrev/
 bug: https://bugs.openjdk.java.net/browse/JDK-8026213
 
 Summary:
 Reflection generates code dynamically to speed up reflection processing 
 after startup. The first
 15 runs of a reflection call  use the vm code path, after that we use the 
 generated code path, which
 needs to use invokespecial on private methods in interfaces.
 
 
 You don't need to pass modifiers as a parameter to emitInvoke since it is 
 set as a field on AccessorGenerator and used, e.g. see isStatic().
 
 So perhaps add the following method to AccessorGenerator:
 
   protected boolean isPrivate() {
   return Modifier.isPrivate(modifiers);
   }
 
 and do:
 
 642 if (isInterface()) {
 
 643 if (isPrivate()) {
 644   cb.opc_invokespecial(targetMethodRef, count, 0);
 645 } else {
 
 646   cb.opc_invokeinterface(targetMethodRef,
 647  count,
 648  count,
 649  
 typeSizeInStackSlots(returnType));
 
 650 }
 
 651 } else {
 652 cb.opc_invokevirtual(targetMethodRef,
 653  count,
 654  
 typeSizeInStackSlots(returnType));
 655 }
 656 }
 657 }
 
 ?
 
 Paul.
 
 Tested:
 Test attached to the bug
 
 Also - all the 8011311 private method testing was run with this in the 
 build:
 Robert Field's TypeTest
 8025475 test
 defmeth privatemethodstest with reflection
 John Rose's intfbug
 jtreg: java.util, java.lang
 jck vm, lang
 
 thanks,
 Karen
 
 
 
 

RFR: Lambda 8026213: Reflection support for private methods in interfaces

[Karen Kinnear]

Please review:

webrev: http://cr.openjdk.java.net/~acorn/8026213/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8026213

Summary:
Reflection generates code dynamically to speed up reflection processing after 
startup. The first
15 runs of a reflection call  use the vm code path, after that we use the 
generated code path, which
needs to use invokespecial on private methods in interfaces.

Tested:
Test attached to the bug

Also - all the 8011311 private method testing was run with this in the build:
Robert Field's TypeTest
8025475 test
defmeth privatemethodstest with reflection
John Rose's intfbug
jtreg: java.util, java.lang
jck vm, lang

thanks,
Karen

Re: static vs. default interface methods and inheritance VM/javac issues

[Karen Kinnear]

Thank you, we really appreciate all testing.

I have a fix in a prototype in the vm for this. Let me know if you want an 
early patch. 
Or you can just file a bug and that way you'll know when the fix is officially 
in the tree.

thanks,
Karen

On Sep 12, 2013, at 10:59 AM, Peter Levart wrote:

 Hi,
 
 While testing behavior of reflection on default and static interface methods, 
 using self-built JDK from latest tip of jdk8/tl, I found:
 
 The following program:
 
 public class DefaultVsStaticInterfaceMethodTest {
 public interface A {
 default void m() {
 System.out.println(A.m() called);
 }
 }
 
 public interface B {
 static void m() {
 System.out.println(B.m() called);
 }
 }
 
 public interface C extends A, B {
 }
 
 public static void main(String[] args) throws Exception {
 C c = new C() {};
 c.m();
 }
 }
 
 
 ...compiles, but gives a runtime error:
 
 Exception in thread main java.lang.AbstractMethodError: Conflicting default 
 methods: DefaultVsStaticInterfaceMethodTest$A.m 
 DefaultVsStaticInterfaceMethodTest$B.m
 at 
 DefaultVsStaticInterfaceMethodTest$1.m(DefaultVsStaticInterfaceMethodTest.java)
 at 
 DefaultVsStaticInterfaceMethodTest.main(DefaultVsStaticInterfaceMethodTest.java:28)
 
 
 A slightly modified program: C extends A, B replaced with C extends B, A:
 
 http://cr.openjdk.java.net/~plevart/jdk8-tl/StaticVsDefaultInterfaceMethods/DefaultVsStaticInterfaceMethodTest.java
 
 ...also compiles, but crashes the VM when run:
 
 #
 # A fatal error has been detected by the Java Runtime Environment:
 #
 #  SIGSEGV (0xb) at pc=0x7fd4d5020bf9, pid=9964, tid=140552419804928
 #
 # JRE version: OpenJDK Runtime Environment (8.0) (build 
 1.8.0-internal-peter_2013_09_12_16_29-b00)
 # Java VM: OpenJDK 64-Bit Server VM (25.0-b48 mixed mode linux-amd64 
 compressed oops)
 # Problematic frame:
 # j  DefaultVsStaticInterfaceMethodTest.main([Ljava/lang/String;)V+9
 #
 # Failed to write core dump. Core dumps have been disabled. To enable core 
 dumping, try ulimit -c unlimited before starting Java again
 #
 # An error report file with more information is saved as:
 # /home/peter/work/git/jdk8-tl/out/production/test/hs_err_pid9964.log
 #
 # If you would like to submit a bug report, please visit:
 #   http://bugreport.sun.com/bugreport/crash.jsp
 #
 Aborted (core dumped)
 
 
 Here's the hs_err_pid file:
 
 http://cr.openjdk.java.net/~plevart/jdk8-tl/StaticVsDefaultInterfaceMethods/hs_err_pid9964.log
  
 
 
 Regards, Peter
 

Re: RFR: 8022547: [verifier] move DefaultMethodRegressionTests.java to jdk

[Karen Kinnear]

Kumar,

Looks good. And thank you - we definitely needed the URLClassLoader so that we
actually test the verifier, unless we had a force -Xverify:all flag. Thank you 
for
fixing the major version from 0x33 -0x34.

I presume the push will also delete the file from the current location :-)

thanks,
Karen

On Aug 7, 2013, at 8:25 PM, Kumar Srinivasan wrote:

 Hello,
 
 Testing this functionality in langtools does not seem to be the appropriate 
 location,
 and the teams have decided to move it to jdk/test/vm/verifier, which seems to 
 be the
 logical place.
 
 I have modified the test to remove testng dependencies, also added a variant 
 which
 involves loading classes via an URLClassLoader which exposed a test bug wrt. 
 the
 class.version.major not being correct, therefore it is useful to have this 
 variant anyway.
 
 The webrev:
 http://cr.openjdk.java.net/~ksrini/8022547/webrev.0/
 
 The bug:
 http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022547
 
 Thanks
 Kumar
 
 
 
 

Re: Code Review Request: More tests for 7184826: (reflect) Add support for Project Lambda concepts in core reflection

[Karen Kinnear]

Vladimir Ivanov has already written tests at the classfile level, which javac 
doesn't generate.
They do not all pass yet, which is why they are not on by default.

Let me know if anyone wants to study the details.
Note that there is already a bug filed to remove the bridging and covariant 
return subsets of tests
since the vm no longer needs to support that. And we know we still need to 
visit in detail
the expected appropriate exceptions that get thrown.

thanks,
Karen

On Jul 25, 2013, at 10:25 AM, Joel Borggrén-Franck wrote:

 Hi Amy,
 
 On Jul 24, 2013, at 3:30 AM, Amy Lu amy...@oracle.com wrote:
 
 Thank you Dan !
 
 Please see my comments inline...
 
 On 7/24/13 5:12 AM, Dan Smith wrote:
 Hi.
 
 Per a request from Joel, I've taken a look at DefaultStaticTestData.  I 
 don't really have the full context here, but I'm assuming that the 
 annotations get translated into tests that guarantee 1) the result of 
 Class.getMethods is exactly (no more -- excepting Object methods -- and no 
 less) those methods named by MethodDesc annotations; and 2) the result of 
 Class.getDeclaredMethods is exactly (no more, no less) those methods that 
 are marked declared=YES.
 
 The expected results seem accurate.  I would personally focus testing more 
 on different inheritance shapes and less on different combinations of 
 (unrelated) method declarations or presence/absence type variables (!?), 
 but it's a valid test in any case.
 
 There ought to be some testing for scenarios that javac won't generate, 
 like conflicting default method declarations.
 
 Testing on javac is out of this scope, it's covered by langtools tests, 
 say test/tools/javac/defaultMethods/
 
 I sort of agree with Dan here. This wouldn't be testing of javac, rather 
 testing that Core Reflection works for combinations that javac doesn't 
 currently emit. However I think that is an excellent candidate for a follow 
 up test, we can address that after these test are finished. I will file a bug 
 for this.
 
 cheers
 /Joel

Re: RFR: 7150256/8004095: Add back Remote Diagnostic Commands

[Karen Kinnear]

Frederic,

Code looks good - actually it looks very clean. Ship it.

Couple of minor comments that don't require re-review:

1. nmtDCmd.hpp/cpp - copyrights 2012 - 2012, 2013

2. jmm.h
  line 213: True is - True if

3. diagnosticFramework.hpp
  Thank you for the comments!
  line 298 rational - rationale

4. diagnosticCommand.cpp
  lines 105/109 - what prints if p._name is null?

thanks,
Karen

On Apr 30, 2013, at 12:26 PM, frederic parain wrote:

 Hi all,
 
 This is a second request for review to add back
 Remote Diagnostic Commands.
 
 This work adds a new platform MBean providing
 remote access to the diagnostic command framework
 via JMX (already accessible locally with the jcmd
 tool).
 
 There's two CR number because this work is made of two
 parts pushed to two different repositories.
 
 JDK changeset CR 7150256
 http://cr.openjdk.java.net/~fparain/7150256/webrev.06/
 
 HotSpot changeset: CR 8004095
 http://cr.openjdk.java.net/~fparain/8004095/webrev.06/
 
 Questions from previous review have been answered
 in initial review threads. Changesets also include
 some minor changes coming from internal audit and
 feedback sent in private e-mails.
 
 However, one issue is still pending: some unit tests
 use a hard coded port number, which could cause test
 failures if several instances of the same test are
 run on the same machine. I propose to postpone the
 fix of this issue after the JDK8 feature freeze
 (leaving for vacations soon, I won't have time to
 fix tests before the feature freeze).
 
 Thanks,
 
 Fred
 
 -- 
 Frederic Parain - Oracle
 Grenoble Engineering Center - France
 Phone: +33 4 76 18 81 17
 Email: frederic.par...@oracle.com

hg: jdk8/tl/jdk: 8010846: Update the corresponding test in test/vm/verifier/TestStaticIF.java

[karen . kinnear]

Changeset: d254a5f9b93f
Author:acorn
Date:  2013-03-27 13:40 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d254a5f9b93f

8010846: Update the corresponding test in test/vm/verifier/TestStaticIF.java
Summary: Remove test flag -XX:-UseSplitVerifier, not supported classfile 52
Reviewed-by: acorn, hseigel

! test/vm/verifier/TestStaticIF.java

hg: jdk8/tl/jdk: 2 new changesets

[karen . kinnear]

Changeset: 4f520ce7ba3f
Author:acorn
Date:  2013-02-13 16:09 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4f520ce7ba3f

8007888: jdk fix default method: VerifyError: Illegal use of nonvirtual
Summary: Recognize VM generated method in old verifier. With 8004967
Reviewed-by: coleenp, acorn
Contributed-by: bharadwaj.yadava...@oracle.com

! src/share/javavm/export/jvm.h
! src/share/native/common/check_code.c

Changeset: e6f34051c60c
Author:acorn
Date:  2013-02-13 16:15 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e6f34051c60c

Merge

Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code

[Karen Kinnear]

Dan,

I had a question on this comment.

Should we fix this in hotspot?

So you mention recent Linux open() documentation.
How does this behave on Solaris and Mac? I assume the library code is shared 
code across platforms.
Also - what is the oldest Linux we support for JDK8?

thanks,
Karen

On Jan 29, 2013, at 6:55 AM, Alan Bateman wrote:

 
 
 
 - I don't think the O_CLOEXEC code is needed in handleOpen, was this copied 
 from HotSpot?
 In the original hotspot code, it has one section to enable the close-on-exec 
 flag for the new file descriptor as the following.
 
#ifdef FD_CLOEXEC
{
int flags = ::fcntl(fd, F_GETFD);
if (flags != -1)
::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
}
#endif
 
 According to the comment, All file descriptors that are opened in the JVM 
 and not specifically destined for a subprocess should have the close-on-exec 
 flag set.  If we don't set it, then careless 3rd party native code might 
 fork and exec without closing all appropriate file descriptors (e.g. as we 
 do in closeDescriptors in UNIXProcess.c), and this in turn might:
 - cause end-of-file to fail to be detected on some file descriptors, 
 resulting in mysterious hangs, or
 - might cause an fopen in the subprocess to fail on a system suffering 
 from bug 1085341.
 
 And the recent open() function (since Linux 2.6.23) already has O_CLOSEXEC 
 flag to enable this flag by default. And it is a preferred way according to 
 the man page,  Specifying this  flag  permits  a program  to  avoid  
 additional fcntl(2) F_SETFD operations to set the FD_CLOEXEC flag.  
 Addi-tionally, use of this flag is essential in some multithreaded programs 
 since using a separate fcntl(2)  F_SETFD  operation to set the FD_CLOEXEC 
 flag does not suffice to avoid race conditions where one thread opens a file 
 descriptor at the same time  as  another  thread  does  a fork(2) plus 
 execve(2).
 Additionally, if O_CLOEXEC is not supported, F_DUPFD_CLOEXEC is preferred 
 than FD_CLOEXEC, which is what hotspot is using right now.
 I don't think we need this because the file descriptor will be closed at exec 
 time anyway (there is logic in Runtime.exec to iterate over the file 
 descriptors and close them). Also we don't do this in other areas of the 
 platform where we use open directly.
 
 

Re: RFR (M): JDK-8004967 - Default method cause java.lang.VerifyError: Illegal use of nonvirtual function call

[Karen Kinnear]

Bharadwaj,

Makes me glad Remi brought up his concerns :-)

I like the additional checking. I wonder if you could possibly modify this to 
rename
the API to JVM_IsVMGeneratedMethodIx - since that might be clearer that this is 
has internal
vm meaning which is not related to any of the specifications on the constant 
pool etc.

That also might be still useful if we were to add additional vm generated 
methods in future
which are not specifically normal vs overpass.

thanks,
Karen

On Jan 18, 2013, at 9:37 AM, Bharadwaj Yadavalli wrote:

 Thanks to Remi, Dean and Karen for looking at the code changes.
 
 Upon further contemplation, it seems to me that my initial proposed change 
 (http://cr.openjdk.java.net/~bharadwaj/8004967/webrev/) *might* incorrectly 
 trust bytecode in methods that were *not* VM generated and flag it as legal.
 
 So, I have implemented, IMHO, a more robust/correct way to handle 
 verification of methods generated by the VM in the old verifier. This change 
 is consistent with  the way verification done on such methods in the (new) 
 split verifier (which I think is appropriate).
 
 The new changes are in hotspot and jdk trees.
 
 Please review the changes at 
 http://cr.openjdk.java.net/~bharadwaj/8004967/alt_impl
 
 Hotspot tree changes : 
 http://cr.openjdk.java.net/~bharadwaj/8004967/alt_impl/hotspot/webrev/
 JDK tree changes : 
 http://cr.openjdk.java.net/~bharadwaj/8004967/alt_impl/jdk/webrev/
 
 I ran JCK tests (vm, lang completed; api running), runThese and vm.quicklist 
 with no regressions.
 
 Thanks,
 
 Bharadwaj
 
 On 1/16/2013 1:35 PM, Bharadwaj Yadavalli wrote:
 Please review the change at 
 http://cr.openjdk.java.net/~bharadwaj/8004967/webrev/
 
 Default interface methods are new in Java 8. VM creates overpass methods in 
 the vtable slots of classes to invoke a default method using invokespecial.
 
 Consequently, invocation of default interface methods (i.e., overpass 
 methods in the VM) via invokespecial is legal and should not be flagged as 
 illegal.
 
 In short, this change allows invocation of default methods of Java 8 using 
 invokespecial.
 
 I ran JCK (vm, lang, api) tests, runThese and vm.quicklist with no new 
 failures.
 
 Thanks,
 
 Bharadwaj
 

Re: RFR (M): JDK-8004967 - Default method cause java.lang.VerifyError: Illegal use of nonvirtual function call

[Karen Kinnear]

Change looks good give I don't know any way for the java library to get the 
internal method type information.

Can you please fix the spelling of implementation in the comment?

thanks,
Karen

p.s. And I think you are ensuring !abstract by doing an exact match of flags.
On Jan 16, 2013, at 1:35 PM, Bharadwaj Yadavalli wrote:

 Please review the change at 
 http://cr.openjdk.java.net/~bharadwaj/8004967/webrev/
 
 Default interface methods are new in Java 8. VM creates overpass methods in 
 the vtable slots of classes to invoke a default method using invokespecial.
 
 Consequently, invocation of default interface methods (i.e., overpass methods 
 in the VM) via invokespecial is legal and should not be flagged as illegal.
 
 In short, this change allows invocation of default methods of Java 8 using 
 invokespecial.
 
 I ran JCK (vm, lang, api) tests, runThese and vm.quicklist with no new 
 failures.
 
 Thanks,
 
 Bharadwaj
 

Re: Review request for 4917309 and 6864003

[Karen Kinnear]

JVM_FindClassFromBootLoader is new with JDK7. Kumar added it.

thanks,
Karen

On Jul 24, 2009, at 11:07 AM, Paul Hohensee wrote:

Can't remove it unless it's fixed in jdk6 as well as 7.  Also, one  
of these days we'll
get around to shipping current hotspot in jdk5 and maybe 1.4.2.  How  
would these

be affected?

paul

Keith McGuigan wrote:


I would prefer that we avoid requiring synchronous pushes (so I  
guess I think we should leave that parameter in for now).


If anything, maybe file a low-priority RFE to remove that parameter  
later?


--
- Keith

Mandy Chung wrote:

David Holmes - Sun Microsystems wrote:

Hi Mandy,

661 JVM_ENTRY(jclass, JVM_FindClassFromBootLoader(JNIEnv* env,
662   const char* name,
663   jboolean  
throwError))


Can't we now drop the throwError parameter altogether?

Yes, I could.  I agree it doesn't need this throwError  
parameter.   I decide to leave it since it helps to avoid the  
synchronized pushes.  JVM_FindClassFromBootLoader is already in a  
promoted build.   I can push the JDK fix and HotSpot fix at the  
same time.  Note that the JDK fix and HotSpot fix are pushed and  
integrated in two different gates and at different time.


If I modify the signature, I would have to push the HS fix first  
(say b68).  Wait until b68 is promoted, then I can push the JDK  
fix in b70.


If you strongly feel that I should drop the throwError parameter,  
I could make the change.


Thanks
Mandy


Pity we can't make a similar fix to the extensions loader ...

Cheers,
David

Mandy Chung said the following on 07/24/09 09:53:
This review request is for both the HotSpot runtime and the core  
libs teams.


Fixed 4917309: (cl) Reduce internal usage of  
ClassNotFoundExceptions during class-loading
Fixed 6864003: Modify JVM_FindClassFromBootLoader to return null  
if class not found


Summary:
o Fix java.lang.ClassLoader to use the new VM entry point
  JVM_FindClassFromBootLoader for load a system class from
  the bootstrap classloader that will reduce the number
  of ClassNotFoundException objects thrown by the application
  class loader by 50%.  The remaining half of the  
ClassNotFoundException
  objects are thrown by the extension class loader which is the  
parent

  of the application class loader.
o ClassLoader.loadClass and ClassLoader.findSystemClass will
  throw ClassNotFoundException as they are specified.
o JVM_FindClassFromBootLoader is currently not used (going to
  used by the java launcher see 6864028). There is no issue
  of changing it to return null instead of throwing CNFE.

Webrev:
 http://cr.openjdk.java.net/~mchung/4917309/hotspot-webrev/
 http://cr.openjdk.java.net/~mchung/4917309/jdk-webrev/


Thanks
Mandy