RFR: 8141564: Convert TraceItables and PrintVtables to Unified Logging

Fri Dec 4 23:00:23 UTC 2015

Hi, Karen,

Thanks for the review. Updated webrev: 
http://cr.openjdk.java.net/~rprotacio/8141564.02/

Since I never listed the tests I ran even the first time, here are all 
of them:

  * jtreg
  * JPRT
  * jck vm, lang, & api/java_lang
  * rbt quick and non-colo (which now includes defmeth)
  * invoke*

Replies inline.

On 12/3/2015 4:56 PM, Karen Kinnear wrote:
> Rachel,
>
> Thank you so much for doing (and redoing) this.
>
> Couple of questions/comments.
>
> 1. thank you so much for the transitive overriding test - not easy to 
> figure out how to generate one!
>
Lois walked me through doing that one :)
> 2. I missed the list of what you have tested for the first web rev and 
> the other tests for this one
> I will assume in parallel with the code review you are running 
> (without tracing)
> vm quick & nocolo, jck vm & lang
> defmeth, invoke*
Yeah, I totally forgot to email what tests I had run. Sorry about that! 
And just learned how to run the jck tests.
>
> There might be value in running your new tests with old tracing vs. 
> new and eyeballing the files - to see if they
> get essentially the same information.
>
Yup!
> 3. klassVtable.cpp lines 154-156 …
> Would it make any sense to do if !klass()->is_array_klass() and then 
> just use the log_develop_debug and
> not do the extra develop_log_is_enabled check? No big deal.
>
That makes sense. Changed as suggested.
> 4. thank you so much for extracting the print_linkage_flags - I should 
> have done that earlier - the information
> just grew incrementally.
>
> 5. klassVtable.cpp line 1047 -
>    I think you want an if (m != NULL) here  before dereferencing to 
> print_linkage_flags.
>    I see we miss that check for other dereferences -feel free to add 
> small checks.
>    Don’t completely skip logic paragraphs except for printing ones.
Well, when I was making my change, we saw that it wouldn't be possible 
for m to be null at that point in the code. There are multiple places 
above it where it is dereferenced already, not to mention the fact that 
it is theoretically initialized to be a valid method. Would you like to 
me to add an assert after the declaration
     assert(m != NULL, "methods can never be null");
just to be safe?

>     Thank you for leaving the null check in fill_in_miranda and 
> print_method_at.
>
> 6. log_vtables
> The logic in update_inherited_vtables is quite complex.
> So the value of allocate_new is not really the right conditional to 
> pass in to determine if there
> was an override.
>    The first paragraph applies both for allocate_new=true and 
> allocate_new=false (see !target_method()->is_package_private())
>    so they would have allocate_new = true AND be overriding.
>
> So, perhaps you could add a boolean that says “overrides” or 
> “overriding” or whatever : default it to false, and set it to true 
> right after the put_method_at line (that iswhere the override occurs) 
> and set it to false in the “else” clause?
> And pass that argument in to log_vtables ? And remove the allocate_new 
> comment from log_vtables as well?
>
Done, too.

Thanks!!
Rachel
> And the comment in the “else” is not accurate since I put the 
> package_private fix in - so removing
> it was a good idea.
>
> many thanks,
> Karen
>
>> On Dec 3, 2015, at 2:23 PM, Rachel Protacio 
>> <rachel.protacio at oracle.com <mailto:rachel.protacio at oracle.com>> wrote:
>>
>> Hi,
>> Thanks for the comments! Updated 
>> webrev:http://cr.openjdk.java.net/~rprotacio/8141564.01/ 
>> <http://cr.openjdk.java.net/%7Erprotacio/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.
>>>
>>> Inhttp://cr.openjdk.java.net/~rprotacio/8141564/src/share/vm/interpreter/linkResolver.cpp.udiff.html 
>>> <http://cr.openjdk.java.net/%7Erprotacio/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 
>>> <http://cr.openjdk.java.net/%7Erprotacio/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 
>>> <http://cr.openjdk.java.net/%7Erprotacio/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 
>>> <http://cr.openjdk.java.net/%7Erprotacio/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/ 
>>>> <http://cr.openjdk.java.net/%7Erprotacio/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
>