> On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
>> Mandy,
>> Webrev updated in-place (press shift-reload).
>>http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14>> getFinalizerHistogram() now returns Entry[]
>> @library and @build removed from the test.
Looks fine.
Thanks
Mandy
>> -Dmitry
>> On 2015-06-04 16:56, Mandy Chung wrote:
>>>>> On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
>>>>>> Mandy,
>>>>>> Thank you for the review.
>>>>>> Updated webrev is:
>>>>>>http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14>>>>>>> Thanks for the update and the test.
>>>>> addressed comments, added a unit test to JDK workspace.
>>>>>>> This test does not need @library and @build, right?
>>>>>>>> On 2015-06-03 21:36, Mandy Chung wrote:
>>>>>>> Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
>>>> The VM knows the returned type anyway.
>>>>>> "[java/lang/ref/Entry" signature would need a separate symbol entry in
>>> swollen vmSymbols::init() so I would prefer to stay with more generic
>>> Object[]
>>>>>>> You already add several symbols - why is it an issue for another one? Or if adding vm symbols is a concern, you should revert to String and int[] that you decide not to. The decision should apply consistently (use String and int, or new Java type).
>>>>>>>> Perhaps the getFinalizerHistogram method should be renamed
>>>> e.g. "dump"?
>>>>>> The line in vmSymbols looks like:
>>>>>> template(
>>> get_finalizer_histogram_name, "getFinalizerHistogram")
>>>>>> I would prefer to keep method name specific enough to be able to
>>> find it by grep in jdk code.
>>>>>> Grep in jdk code is easy as you have a new class :)
>>>> Mandy
>>>>>>>> (other comments are addressed)
>>>>>> -Dmitry
>>>>>>>>> On 2015-06-03 21:36, Mandy Chung wrote:
>>>>>>>>>>>> On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
>>>>> Updated webrev:
>>>>>>>>>>http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13>>>>>>>> I reviewed the jdk change. The version looks okay and some comments:
>>>>>>>> ReferenceQueue.java - you should eliminate the use of rawtype:
>>>>>>>> 84 Reference rn = r.next;
>>>>>>>> Change Reference to Reference<? extends T>
>>>>>> done.
>>>>>>>>>>> The forEach method - eliminate the use of raw type and
>>>> move @SuppressWarning to the field variable in line 182:
>>>>>>>> @SuppressWarnings("unchecked")
>>>> Reference<? extends T> rn = r.next;
>>>>>> done.
>>>>>>>>>>> FinalizerHistogram.java
>>>> Copyright year needs update.
>>>>>> done.
>>>>>>>> I personally think the VM code would be much simpler if you stay with
>>>> alternative entry of String and int than dealing with a
>>>> FinalizerHistogramEntry private class. It's okay as FinalizerHistogram
>>>> class is separated.
>>>>>>>> The comment in line 35 is suitable to move to the class description and
>>>> this is the suggestion.
>>>>>>>> // This FinalizerHistogram class is for GC.finalizer_info diagnostic
>>>> command support.
>>>> // It is invoked by the VM.
>>>>>> done.
>>>>>>>>>>> Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM
>>>> knows the returned type anyway.
>>>>>> "[java/lang/ref/Entry" signature would need a separate symbol entry in
>>> swollen vmSymbols::init() so I would prefer to stay with more generic
>>> Object[]
>>>>>>> It's an inner class of
>>>> FinalizerHistogram and it can simply be named as "Entry". I suggest to
>>>> have Entry::increment method to replace ".instanceCount++".
>>>>>> done.
>>>>>>>>>>> The tests are in hotspot/test. Can you add a unit test in jdk/test?
>>>> Perhaps you can test FinalizerHistogram.getFinalizerHistogram() via
>>>> reflection - just a sanity test.
>>>>>> done. The test repeats hotspot one, but uses reflection instead of VM call.
>>>>>>>>>>> A minor naming comment - now you have a FinalizerHistogram class.
>>>> Perhaps the getFinalizerHistogram method should be renamed e.g. "dump"?
>>>>>> The line in vmSymbols looks like:
>>>>>> template(
>>> get_finalizer_histogram_name, "getFinalizerHistogram")
>>>>>> I would prefer to keep it specific enough to be able to
>>> find it by grep in jdk code.
>>>>>>>>> --
>>> Dmitry Samersoff
>>> Oracle Java development team, Saint Petersburg, Russia
>>> * I would love to change the world, but they won't give me the sources.
>>>>> --
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * I would love to change the world, but they won't give me the sources.