Request for review: JDK-8004728 Hotspot support for parameter reflection

[Eric McCorkle]

Thanks to all for their reviews.  There is an updated version here:

http://oklahoma.us.oracle.com/~cphillim/webrev/JDK-8004728/

On 01/07/13 14:42, Coleen Phillimore wrote:
> 
> 
> John, Thank you for your thorough review of this.   I have a couple of
> comments below.
> 
> On 01/05/2013 06:41 PM, John Rose wrote:
>> On Jan 4, 2013, at 6:14 PM, Eric McCorkle wrote:
>>
>>> Please review the following change set, which implements support for
>>> method parameter reflection in hotspot.  The open webrev can be found
>>> here:
>>>
>>> http://cr.openjdk.java.net/~acorn/8004728/webrev
>> Here are some more comments.
>>
>> The method java_lang_reflect_Parameter::index uses modifiers_offset,
>> which isn't correct.  I don't see any other errors like that.  It
>> would be best to test such things with asserts or even unit tests,
>> although code this simple can usually be verified by inspection.
>>
>> I notice that the Parameter class is marked "Opt", which is good.  In
>> order to avoid surprise JVM crashes during testing, it should be the
>> case that the changed JVM will work correctly with old versions of the
>> JDK which lack Parameter.class, even with arbitrary inputs.  I think
>> Murphy's Law will bite if the JVM happens to load any class file that
>> contains the new attribute.  I suggest providing for this more
>> explicitly:
>>
>> +    } else if (method_attribute_name ==
>> vmSymbols::tag_method_parameters()) {
>> +      method_parameters_length = cfs->get_u1_fast();
>> +      method_parameters_data = cfs->get_u1_buffer();
>> +      cfs->skip_u2_fast(method_parameters_length);
>> +      cfs->skip_u4_fast(method_parameters_length;
>> ++    if (!SystemDictionary::Parameter_klass_loaded())
>> ++      method_parameters_length = 0;  // ignore this attribute if it
>> cannot be reflected
>>
>> I think that's the only point necessary for such a check.  (This path
>> could be tested by manually removing Parameter.class from an rt.jar
>> and running unit tests, or by injecting a typo into the name of the
>> class in vmsymbols.hpp.)  In general, version numbers, etc., will keep
>> such bugs away, but it is better to be safe than sorry.
>>
>> In jvm.cpp and reflection.cpp there is some ambivalence about the
>> method pointer argument for a Parameter factory method.  Since the
>> term "executable" is foreign to the JVM, and we just use the term
>> "method" everywhere, I suggest using the word "executable" (confusing
>> in the context of the JVM, and non-existent in reflection.cpp) only
>> for the field in the Parameter itself (in javaClasses.hpp etc.).  Use
>> the word "method" elsewhere.  The distinction between the JNI method
>> argument and the JVM handle is often conveyed by a simple conventional
>> prefix or suffix.  Same general comment for the distinction between a
>> reflective object and an internal metadata pointer.  For example:
>>    +JVM_ENTRY(jobjectArray, JVM_GetMethodParameters(JNIEnv *env,
>> jobject method_jh))
>> +{
>> +  JVMWrapper("JVM_GetMethodParameters");
>> +  // method is a handle to a java.lang.reflect.Method object
>> +  Handle reflected_method(THREAD,
>> JNIHandles::resolve_non_null(method_jh));
>> +  Method* method_oop = jvm_get_method_common(method, CHECK_NULL);
>> +  methodHandle method(THREAD, method_oop);
>>
>> Please avoid leading underscores in local variable names, so change _r
>> to result_oop and _m to method_oop, or some such.  There are
>> pre-existing examples of this pattern in jvm.cpp, for example in
>> JVM_DoPrivileged and JVM_FindClassFromClass.  We use leading
>> underscores only in limited ways, because of portability concerns.
> 
> I agree, except method_oop isn't an oop, so method_pointer or golly,
> even m would be a better name.
>> The subroutine jvm_get_method_common no longer needs to take a TRAPS
>> argument, since the permgen removal.  You can clean that up since you
>> are in the same file, if you want.  (There will be lots of little nits
>> like this, after permgen removal.  They can be adjusted on the fly, as
>> we touch code.)  Removing the "TRAPS" will make it possible to
>> simplify your new code, since you won't need a CHECK_NULL and the
>> extra method_oop temporary.
> 
> 
> If Eric takes out the "TRAPS" parameter to get_method_common, he also
> needs to change these lines since TRAPS gives you THREAD.   I still
> think it's a nice thing to do.
> 
>   KlassHandle kh(THREAD, k);
>   Method* m = InstanceKlass::cast(kh())->method_with_idnum(slot);
> to
>   Method* m = InstanceKlass::cast(k)->method_with_idnum(slot);
>>
>> The changes in constMethod.cpp look correct, but it's hard to tell. 
>> The code which manages the "inlined tables" is delicate and very
>> sensitive to initialization ordering.  You might try fault injection
>> to ensure that the asserts will catch bugs.  By that I mean moving the
>> insertion around a bit in 'set_inlined_tables_length' and ensuring
>> that you get an error diagnostic each time the code is out of place. 
>> Add asserts as needed.
>>
>> (Side note: I have some qualms about constMethod which are not due to
>> your changes, but your changes expose a scaling problem in the coding
>> pattern used.  It appears to me that, given N inline tables, there are
>> N(N+1)/2 code paths that need to be written correctly, and that a bug
>> on any of those paths may stay latent until the exact pattern of
>> inline-table bits shows up in practice.  One symptom of this is the
>> increasing levels of indentation in the logic (which could be
>> clarified slightly by else-if chaining, BTW).  Not sure what to do
>> about this.  I don't think there's a quick incremental fix.  If we go
>> after metadata footprint, I think all such metadata (if not frequently
>> accessed) should be stored via compressed streams
>> (CompressedWriteStream etc.), and that will provide a suitable moment
>> to address this problem, but it's not in scope for this fix.)
> 
> I was also wondering if there was a better way to inline these optional
> tables in constMethod because I want to add the method annotations to
> each method (remove from Annotations* type).
> 
> Thanks,
> coleen
>> About whitespace:  I was reviewing using the raw patch file, and saw a
>> number of apparent oddities involving whitespace.  They don't affect
>> correctness, but it would be good to tidy them, to control excess
>> noise in our code base.  For example:
>>
>>     template(java_lang_reflect_Field,                  
>> "java/lang/reflect/Field")                  \
>> +  template(java_lang_reflect_Parameter,              
>> "java/lang/reflect/Parameter")                  \  <== extra indent here
>>
>> -                                      compressed_line_number_size,
>> +                   compressed_line_number_size,  <== changed indent
>> here, with no apparent value add
>>
>>                                         checked_exceptions_len,
>> +                      method_parameters_len,  <== one space extend,
>> probably a diff+tab artifact
>>                                         generic_signature_index,
>>
>> +  if(has_method_parameters()) {  <== missing recommended space after
>> 'if'
>>
>> +    return has_generic_signature() ? (last_u2_element() - 1) :
>> +      last_u2_element();  <== tiny indent, should be horizontally
>> aligned with previous expression, as with similar code nearby
>>
>>
>> It's hard to tell for sure, since there are tabs in the diff, which a
>> pre-push "jcheck" will reject.
>>
>> Although we (or our text editors) sometimes re-indent code we touch,
>> it is often better to follow the ambient indentation, if the change
>> would not improve readability or consistency.  If you were re-aligning
>> parameter declarations, that's a reasonable improvement; I'm not sure
>> that's what you did because of the tabs.  In some places you broke a
>> parameter list onto multiple lines, which is fine.
>>
>> I updated the public style guide to clarify some of these points:
>>    https://wikis.oracle.com/display/HotSpotInternals/StyleGuide
>>
>> Thanks for making these changes.  I think they will be taken up
>> quickly by language implementors.
>>
>> — John
>>
>