RFR: 8141564: Convert TraceItables and PrintVtables to Unified Logging

Coleen Phillimore coleen.phillimore at oracle.com
Thu Jan 7 18:36:56 UTC 2016 

This still looks good!  I think I interpreted David's comment to not 
require another round, so I think he's a reviewer.

Coleen

On 1/7/16 12:15 PM, Rachel Protacio wrote:
> Hi, David,
>
> Thanks for that. I've corrected the issue in interpreterRuntime.cpp as 
> well as in klassVtable.cpp. Did you look at all the code - shall I 
> count you as a reviewer?
>
> Updated webrev: http://cr.openjdk.java.net/~rprotacio/8141564.04/
>
> Rachel
>
> On 12/8/2015 11:05 PM, David Holmes wrote:
>> Hi Rachel,
>>
>> src/share/vm/interpreter/interpreterRuntime.cpp
>>
>> The ResourceMark is now used outside of the actual logging code. If a 
>> ResourceMark is needed then I think we need to use the "if 
>> (log_is_enabled(...)" form.
>>
>> Thanks,
>> David
>>
>>
>>
>> On 9/12/2015 4:46 AM, Rachel Protacio wrote:
>>> Hi,
>>>
>>> On 12/8/2015 12:28 PM, Karen Kinnear wrote:
>>>> Rachel,
>>>>
>>>> Thank you for the updates.
>>>> Can you please sanity check one thing for me - the log_tables boolean
>>>> “overrides” appears to
>>>> be set correctly in the callers, to mean “yes, this overrides”.
>>>> In log_vtables, is it possible the choice of what to print is 
>>>> reversed?
>>>>
>>>> And perhaps add a check to your tests for example that “NOT
>>>> overriding” catches the right case?
>>> Oh, good catch. Thank you - this is fixed now.
>>>>
>>>> And yes please on the assertion (I have seen null method bugs in
>>>> development and this code is an early
>>>> opportunity to catch that problem).
>>>>
>>>> I don’t need to see an updated web rev and you don’t need to rerun all
>>>> the tests to fix
>>>> the printed string.
>>> Great. Thanks again, Karen and Coleen! Will commit the change.
>>> Rachel
>>>> thanks,
>>>> Karen
>>>>
>>>>> On Dec 7, 2015, at 1:37 PM, Coleen Phillimore
>>>>> <coleen.phillimore at oracle.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 12/4/15 6:00 PM, Rachel Protacio wrote:
>>>>>> 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?
>>>>> In the code here:
>>>>> http://cr.openjdk.java.net/~rprotacio/8141564.02/src/share/vm/oops/klassVtable.cpp.udiff.html 
>>>>>
>>>>> <http://cr.openjdk.java.net/~rprotacio/8141564.02/src/share/vm/oops/klassVtable.cpp.udiff.html> 
>>>>>
>>>>>
>>>>>
>>>>> Yes, Method* m = method->at(i); can never return NULL or else the
>>>>> whole VM would crash everywhere.  I suspect the logging code here was
>>>>> copied from other instances where 'm' could be null. This line
>>>>> would crash first:
>>>>>
>>>>> if (interface_method_needs_itable_index(m)) {
>>>>>
>>>>>
>>>>> Can you clean up this line also to not check for NULL:
>>>>>
>>>>> const char* sig = (m != NULL) ? m->name_and_sig_as_C_string() :
>>>>> "<NULL>";
>>>>>
>>>>>
>>>>> And add the assert if Karen agrees if it's needed.
>>>>>
>>>>> The rest of the changes look good.   I don't need to see another
>>>>> revision.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>>>     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>
>>>>>>>> <mailto: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/~rprotacio/8141564.01/><http://cr.openjdk.java.net/%7Erprotacio/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 
>>>>>>>>>
>>>>>>>>> <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 
>>>>>>>>>
>>>>>>>>> <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/~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 
>>>>>>>>>
>>>>>>>>> <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/~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 
>>>>>>>>>
>>>>>>>>> <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/~rprotacio/8141564/test/runtime/logging/p1/A.java.html><http://cr.openjdk.java.net/%7Erprotacio/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/~rprotacio/8141564/>
>>>>>>>>>> <http://cr.openjdk.java.net/%7Erprotacio/8141564/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Erprotacio/8141564/>>
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8141564
>>>>>>>>>> <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
>>>
>

More information about the hotspot-runtime-dev mailing list