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

[Coleen Phillimore]

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
>