RFR: 8141564: Convert TraceItables and PrintVtables to Unified Logging
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Jan 6 22:14:40 UTC 2016
I remember this change! This came out very nice. I agree that
@ignore'ing the test until we figure out what's wrong with these tests
on Windows is a good thing to do.
Thanks,
Coleen
On 1/6/16 4:31 PM, Rachel Protacio wrote:
> Hi,
>
> When I tried to commit this change, the ItablesTest was failing on
> windows, so I've been trying to get to the bottom of that failure. As
> other logging tests have recently proved themselves flaky on windows
> as well, it seems to be a deeper issue. As such, I have added the
> problem to https://bugs.openjdk.java.net/browse/JDK-8146435 for more
> thorough investigation and ignored the ItablesTest for the time being.
>
> Here's my new webrev http://cr.openjdk.java.net/~rprotacio/8141564.03/
> which only differs from the previously approved code via the @ignore
> line. The code passes JPRT.
>
> Thank you!
> Rachel
>
> 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?
>>
>> 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.
>>
>> 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