RFR: 8141564: Convert TraceItables and PrintVtables to Unified Logging

[Rachel Protacio]

Hi,
Thanks for the comments! Updated webrev: 
http://cr.openjdk.java.net/~rprotacio/8141564.01/ (Builds and passes 
logging jtreg tests.)

Replies inline.

On 12/2/2015 2:34 PM, Coleen Phillimore wrote:
> Rachel, There are a lot of changes here.  I have several comments and 
> more suggested cleanup that should be done with this work.
>
> In 
> http://cr.openjdk.java.net/~rprotacio/8141564/src/share/vm/interpreter/linkResolver.cpp.udiff.html
>
> I did the original refactoring for trace_method_resolution.  What I 
> think would be better is to make it an internal static function in 
> linkResolver.cpp and not defined in the linkResolver.hpp.  Make the 
> call unconditional and pass vtable index in the last parameter as a 
> default parameter with default value -1 so the last line in the 
> function is the tty->cr() that I had outside the function call (that 
> was pretty icky).  And put all the ResourceMark and logstream logic 
> inside the trace_method_resolution function.  It would never print to 
> tty so doesn't need an outputStream parameter for other callers.
>
> So there would be only the one line trace_method_resolution(params) in 
> the regular code.
Thanks for the advice! I still have to call the function from within the 
conditional because it's develop level only and because I need to 
specify in the call whether it's for itables or vtables. Otherwise, I've 
followed your suggestions.
>
> http://cr.openjdk.java.net/~rprotacio/8141564/src/share/vm/logging/logTag.hpp.udiff.html 
>
>
> Can you put these tags near each other?
>
I have them sorted alphabetically, which I think is more useful in the 
long run.
> http://cr.openjdk.java.net/~rprotacio/8141564/src/share/vm/oops/klassVtable.cpp.udiff.html 
>
>
> One of the things that Markus Gronlund's made me think that we should 
> do with his cleanup is to stop declaring functions as members of 
> classes if they don't need external linkage.  From his RFR(L): 
> 8140485: Class load and creation clean up:
>
> "Comment: In my opinion, we should attempt to break the pattern of 
> having private functions declared in header files when the private 
> function does not need to reach inside "this"."
>
> I think the function log_vtables should follow this and be declared as 
> a static internal function in klassVtable.cpp above the function where 
> it's used.
>
> It's true that you don't see this very much in the hotspot sources.
>
> The refactoring of log_vtables is good and very much appreciated.
>
> In each of the logging blocks of code, there are about 6 lines that 
> are about the same, that print the access flags and whether it's a 
> default or overpass method.  (Karen and Lois should give you a TOI 
> about what that means!)  Can you add a method to method.hpp that's 
> something like print_flags(outputStream* st) and move this printing 
> there, and make those blocks call your new function?  That would 
> shorten many of these logging blocks of code and would be nice.  You 
> could call this from the linkResolver.cpp trace_method_resolution() 
> function too.
Done.
>
> http://cr.openjdk.java.net/~rprotacio/8141564/test/runtime/logging/p1/A.java.html 
>
>
> These classes have missing copyrights.
Fixed.
Thanks!
Rachel
>
> Thanks!
> Coleen
>
>
> On 12/1/15 5:58 PM, Rachel Protacio wrote:
>> Hi,
>>
>> Please review this changeset that converts -XX:+TraceItables and 
>> -XX:+PrintVtables flags to -Xlog:itables and -Xlog:vtables.
>>
>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8141564/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8141564
>>
>> In case it's not clear, the changes in oops/klassVtable.cpp around 
>> 462-498 are a refactoring of the duplicate code from a conditional to 
>> a function with a boolean argument.
>>
>> Thank you,
>> Rachel
>