Review request: JMC-6127: Live Objects Page do not forward selections correctly

Guru guru.hb at oracle.com
Tue Nov 27 17:28:25 UTC 2018


Thanks Henrik and Marcus,

Please find Updated webrev : http://cr.openjdk.java.net/~ghb/JMC-6127/webrev.1/ <http://cr.openjdk.java.net/~ghb/JMC-6127/webrev.1/> 

-Guru

> On 27-Nov-2018, at 9:17 PM, Henrik Dafgård <hdafgard at gmail.com> wrote:
> 
> Hi,
> 
> The leaves in the aggregated reference trees are not necessarily leaks, they're just known to be alive when the recording is dumped and the documentation should reflect that. I think referring to the leaf nodes as closer to OldObjectSamples or something similar would be better since it differentiates them more clearly from the leak candidates.
> 
> There also seems to be a change in MemoryLeakPageUI constructor where the initial state of the table is set to display model.getLeakObjects() instead of model.getRootObjects(), which should probably be configurable. But there is also the case where selecting a time range will show model.getLeakObjects(IRange) and then unselecting a time range will revert to model.getRootObjects() even in this change.
This is intended, “model.getRootObjects()” are required if there is no time line selected in the chart, where as “model.getLeakObjects(IRange)” returns RootObjects in which the alocationTime is filtered based on its leaf node rather than its Root.
> 
> * A helper method to calculate number of leaked object with in specified time
This is taken are with updated webrev.1.
> Within is one word, and since this is a sentence should be followed with a period.
> 
> Other than that it looks good.
> 
> // Henrik Dafgård
> 
> 
> On Tue, 27 Nov 2018 at 15:00, Marcus Hirt <marcus.hirt at oracle.com <mailto:marcus.hirt at oracle.com>> wrote:
> Hi Guru,
> 
> Looks good, but documentation could be a bit improved e.g. on the 
> ReferenceTree related classes. It should not say "Object which is leaked" 
> rather "referenced object" or something similar. Also, it should preferably 
> explain what the method does, and not only list the parameter. For example:
> 
>           * @param leafNode
>           *            Object which is leaked
>           */
> 
Please check the updated webrev.1. 
> Kind regards,
> Marcus
> 
> From: Guru <guru.hb at oracle.com <mailto:guru.hb at oracle.com>>
> Date: Tuesday, 27 November 2018 at 07:59
> To: <jmc-dev at openjdk.java.net <mailto:jmc-dev at openjdk.java.net>>, Marcus Hirt <marcus.hirt at oracle.com <mailto:marcus.hirt at oracle.com>>
> Subject: Review request: JMC-6127: Live Objects Page do not forward selections correctly
> 
> Hi All,
> 
> Please review the fix for 
> JBS : https://bugs.openjdk.java.net/browse/JMC-6127 <https://bugs.openjdk.java.net/browse/JMC-6127>
> Webrev : http://cr.openjdk.java.net/~ghb/JMC-6127/webrev.0/ <http://cr.openjdk.java.net/~ghb/JMC-6127/webrev.0/>
> 
> Root cause and its solution is updated in JBS. 
> 
> Thanks
> Guru
> 
> 



More information about the jmc-dev mailing list