Re: [mat-dev] Proposal: MAT Warning System

[Kevin Grigorenko <kevin.grigorenko@xxxxxxxxxx>]

Hey Krum,

> to me WarningResolver and WarningResolution sounds as we are already solving the problems J May be just because of my non-native English. Shall we go for another name, or is this one fine?

That's a good point. I took resolver from the name resolver, but you're right that it seems like it's actually resolving the warning. How about WarningInvestigator and WarningInvestigation?

> I think we have several options:
> #1          Assuming that we introduce the functionality post 1.3, we could go for a major version jump afterwards (i.e. mat 2.0) and signal that there are incompatible changes. > It is then clearer, but still this doesn’t help any adopters.
> #2          Add another interface, and make out object implementation implement both – wound need some casting on all the places we would like to use the warnings
> #3          Use some kind of singleton to pass the object and get the warnings - we will need one anyway (at least internally) to read all extensions.

I'm open to any of them. I like #1 or #2 because then at least the warning system is consistent with the name resolver system. If we choose #2, then that means that we would have to do both an instanceof and then a cast each time right? Do you think this could impact performance? We'll only be running the warning investigations on each displayed object, so I don't expect it would add too much.

By the way, that brings up the point of performance impact analysis: do we have any "standardized" test where we can run a performance test on the various parts of MAT with a dump so that when we make a big change to MAT we can do a performance regression test? Ideally this would be done on some unchanging build system so that the hardware is the same each time.

> There are still some open questions, but I guess most of them can be answered while developing and playing with the new feature.

Great! I'll wait on the answer to the compatibility point and also if Andrew has any more thoughts on the previous mail, and then I'll start coding...

--
Kevin Grigorenko
IBM WAS SWAT Team
kevin.grigorenko@xxxxxxxxxx
Blog: https://www.ibm.com/developerworks/mydeveloperworks/blogs/kevgrig/?lang=en


"Tsvetkov, Krum" ---03/15/2013 06:15:19 AM---Hi Kevin, - I think we collected enough ideas to justify the creation of the per-object checks

From:	"Tsvetkov, Krum" <krum.tsvetkov@xxxxxxx>
To:	Memory Analyzer Dev list <mat-dev@xxxxxxxxxxx>,
Date:	03/15/2013 06:15 AM
Subject:	Re: [mat-dev] Proposal: MAT Warning System
Sent by:	mat-dev-bounces@xxxxxxxxxxx

---

Hi Kevin,
 
- I think we collected enough ideas to justify the creation of the per-object checks
 
- I still think we should be very careful with performance, but it is difficult to judge before having real experience
 
> I worry about running the scan during the initial open since dump open time is already a big complaint about MAT. I also worry about making it some button that you have to click since most users won't click it. I like the approach of IClassSpecificNameResolver, although maybe we should add a SoftReference cache of the resolved warning results?
 
Let’s drop the idea for now. I would also propose that we don’t introduce a new cache. We can still add it later, If we see that it is needed 
 
- to me WarningResolver and WarningResolution sounds as we are already solving the problems J May be just because of my non-native English. Shall we go for another name, or is this one fine?
 
>> Adding methods to IObject is not a compatible change

> What's the alternative? If I have an IObject and I want to get the list of warnings for it, would I have to use some sort of singleton service?
I think we have several options:

-          Assuming that we introduce the functionality post 1.3, we could go for a major version jump afterwards (i.e. mat 2.0) and signal that there are incompatible changes. It is then clearer, but still this doesn’t help any adopters.
-          Add another interface, and make out object implementation implement both – wound need some casting on all the places we would like to use the warnings
-          Use some kind of singleton to pass the object and get the warnings - we will need one anyway (at least internally) to read all extensions.

I am still not sure which way is the best…
 
There are still some open questions, but I guess most of them can be answered while developing and playing with the new feature. I guess we should agree on how to introduce the functionality (i.e. the last compatibility related point above).
 
Regards,
Krum
 
From: mat-dev-bounces@xxxxxxxxxxx [mailto:mat-dev-bounces@xxxxxxxxxxx] On Behalf Of Kevin Grigorenko
Sent: Montag, 11. März 2013 23:04
To: Memory Analyzer Dev list
Subject: Re: [mat-dev] Proposal: MAT Warning System

Hey Andrew, Krum,

Sorry for the delay, I was on vacation. Great feedback. Here are some more comments:

> In the last days I also thought that we should first collect some more concrete examples of what problems we could tackle.

Some other ideas:
- For the WebSphere Application Server product, I'd like to add a warning when a thread has been marked as potentially hung by the hung thread detection - just a boolean check.
- WAS Classloader marks that an application is stopped but it hasn't been garbage collected (somebody is holding on to something that they shouldn't) - just checking a field on the ClassLoader.
- Check if various WAS caches are full - just comparing two integer fields.

> it may help to run a scan once, and save the results somehow, so that opening a view later does not result in new computations.

I worry about running the scan during the initial open since dump open time is already a big complaint about MAT. I also worry about making it some button that you have to click since most users won't click it. I like the approach of IClassSpecificNameResolver, although maybe we should add a SoftReference cache of the resolved warning results?

> For example warning about closed files or disposed graphics objects might make sense if they were being retained when no longer needed, but in another situation excessive numbers of unclosed files could indicate a resource leak.

Yes, I could also think of some situations where a warning only occurs in some situations (for example, one large HTTP session is okay, but a thousand is probably not), but I would rather the default set be speedy. What about a resolver marking itself using an annotation which predicts the impact (e.g. @Impact(ImpactLevel.High))? Then there could be a preference that controls the threshold of resolvers to run.

> A standard Eclipse way is to use an overlay (a decorator) on the icon. For example a warning triangle or red error cross on projects which are in error. Would that be more intuitive?

That is probably better. I can try that first but I'd like to see if it's too subtle or not.

> Should we then have a problems view? You could then show warnings/errors on a selection or across the dump, and group the errors by type, then jump to an instance of the error.

Yes, that makes sense. Can we just reuse the existing Problems View or would we have to create or own?

> We could also allow the user to annotate an object with some text (that would be visible in other views), or possibly tasks (e.g. TODO items for objects the user wants to hand inspect).

Do you mean that a user could right click on an object and add a note or TODO marker?

> We could also have a preferences page - should there be a per snapshot preferences for warnings? How might that be done - should that be like per project Java warnings.

What's the benefit of a per-snapshot preference?

> Should snapshots map to Eclipse projects in the projects explorer?

What's the benefit?

> Should we make it easy to have multiple workspaces for MAT? E.g. all snapshots for one end-user application could be in one workspace, and then by switching workspaces you could then see all the snapshots from another application.

Interesting, I rarely use MAT in the process of development, but instead on post-mortem dumps and I usually don't have the matching source code, so I hadn't thought about this use case.

> Would showing details of the warning in a tab in the object inspector be a good location?

That certainly makes sense to add, but I wouldn't want that to be the only place (marker or color I think is the primary notification).

> Would an extension point to specify the icon for an object be useful?

Could this be one of the fields returned in the WarningResolution struct? Does a custom icon conflict with the IMarker/Problems View approach?

> The IClassSpecificWarningResolver seems a reasonable start, though markers seem a good model and could allow the linking of help.

Do you think IClassSpecificWarningResolver should extend IMarker?

> The name resolver implementation has a slight problem that only one resolver per class is allowed.

I think this is because IObject.getClassSpecificName() returns a single String instead of a List. This is one reason I suggested List<WarningResolution> to support multiple warning resolvers for a single class.

> Adding methods to IObject is not a compatible change

What's the alternative? If I have an IObject and I want to get the list of warnings for it, would I have to use some sort of singleton service?

> Why tree view and not table view as well?

Yes, table view should also show it. Your suggestion of using IQueryPart.IS_IMPORTANT and coloring the row seems to be the way to go.

> How do we save/display the warning text in HTML for saved reports?

Good question. How about footnotes that link to the bottom of the HTML?

> @Subject("java.lang.Object") would probably do the same as "*".

I didn't know @Subject supported "instanceof" resolution. So if I have class B which extends class A, and I write a name resolver for class A but I have an instance of class B, then the name resolver of class A will run?

--
Kevin Grigorenko
IBM WAS SWAT Team
kevin.grigorenko@xxxxxxxxxx
Blog: https://www.ibm.com/developerworks/mydeveloperworks/blogs/kevgrig/?lang=en


"Tsvetkov, Krum" ---03/01/2013 04:34:06 AM---I also think it would be a nice improvement to MAT to add some more automatically shown warnings. >>

From:	"Tsvetkov, Krum" <krum.tsvetkov@xxxxxxx>
To:	Memory Analyzer Dev list <mat-dev@xxxxxxxxxxx>,
Date:	03/01/2013 04:34 AM
Subject:	Re: [mat-dev] Proposal: MAT Warning System
Sent by:	mat-dev-bounces@xxxxxxxxxxx

---

I also think it would be a nice improvement to MAT to add some more automatically shown warnings.
 
>> I think we can split the problem into 4 parts.
+1
 
>> 1.Examples of warnings
 
In the last days I also thought that we should first collect some more concrete examples of what problems we could tackle.
My biggest concern about doing some analysis per object is the performance. Let’s try to collect some ideas of concrete warnings, which can be a) calculated based  on (or starting with) a single object and b) calculated very fast. I think the example from Kevin with the big object, and from Andrew with the disposed graphics are a good start. What else could we collect?

-          Big objects (> % of the total heap)
-          Closed files
-          Un-/disposed widgets https://bugs.eclipse.org/bugs/show_bug.cgi?id=274369
-          Small Strings objects, keeping a huge char[] (as result of substring() call)
-          A big, empty collection ?? - not sure if this is useful on a single object. The reports showing millions of empty collections are good, but on a single object I don’t know.

We had some ideas in the past to show WeakHashMap objects, which contain entries where the value is referencing (may be indirectly) the key. This however requires some path analysis, and is one of the warnings which I wouldn’t calculate for every opened view containing a WeakHashMap.
 
>> 2.How should the warnings be indicated to the user?
I find both proposals good - coloring the text or using a marker on the icon.
>> Would showing details of the warning in a tab in the object inspector be a good location?
+1
 
>> Would an extension point to specify the icon for an object be useful?
I think so.
 
I’ll think a bit more about the other questions.
 
 
About performance:
 
Depending on the concrete problems we identify in 1) it may help to run a scan once, and save the results somehow, so that opening a view later does not result in new computations.
I wouldn’t do it in the parsing phase, or at least it should be possible to turn such additional analysis off.
It covers to some extent what Kevin proposed:
>> An additional report could exist on the Overview screen under Reports named "Show Any Warnings." This would require finding all warningResolver extensions and passing each instance of the subject classes to check for warnings.
 
And it is also similar to the current component reports, but there we do not persist the results to mark objects afterwards.
 
I enjoy this discussion, let’s go on J
 
Krum
 
 
 
From: mat-dev-bounces@xxxxxxxxxxx [mailto:mat-dev-bounces@xxxxxxxxxxx] On Behalf Of Andrew Johnson
Sent: Freitag, 1. März 2013 11:50
To: Memory Analyzer Dev list
Subject: Re: [mat-dev] Proposal: MAT Warning System
 
I agree that adding more automatic warnings would make MAT more useful for the inexperienced user. 

As is, I don't think it is quite the right approach. I think we can split the problem into 4 parts. 

1.Examples of warnings - so we can see if warnings are globally applicable, or are complex to calculate. E.g. the warning is per object, so the evaluation needs to be speedy, but also accurate. For example warning about closed files or disposed graphics objects might make sense if they were being retained when no longer needed, but in another situation excessive numbers of unclosed files could indicate a resource leak. 

2.How should the warnings be indicated to the user? 
The coloured coded row (background or foreground?) seems a convenient way to indicate a problem, but there are others. 
See also QueryPart IS_IMPORTANT which highlights a section in a report, and the focus row in blue in a report. 
A standard Eclipse way is to use an overlay (a decorator) on the icon. For example a warning triangle or red error cross on projects which are in error. Would that be more intuitive? Should we then have a problems view? You could then show warnings/errors on a selection or across the dump, and group the errors by type, then jump to an instance of the error. These are markers - see IMarker. We could also allow the user to annotate an object with some text (that would be visible in other views), or possibly tasks (e.g. TODO items for objects the user wants to hand inspect). We could also have a preferences page - should there be a per snapshot preferences for warnings? How might that be done - should that be like per project Java warnings. Should snapshots map to Eclipse projects in the projects explorer? Should we make it easy to have multiple workspaces for MAT? E.g. all snapshots for one end-user application could be in one workspace, and then by switching workspaces you could then see all the snapshots from another application. 
Would showing details of the warning in a tab in the object inspector be a good location? 
Would an extension point to specify the icon for an object be useful? 

3.How should adopters write warning extensions? 
The IClassSpecificWarningResolver seems a reasonable start, though markers seem a good model and could allow the linking of help. 

4.How should it be implemented internally? 
Doing icon overlays in org.eclipse.mat.api or report is harder, as currently they does not use the Eclipse UI classes. That was important when AIX didn't have a 64-bit UI, but might be less so now. 

Other comments: 

1.Internals/adopters 
The name resolver implementation has a slight problem that only one resolver per class is allowed. Name resolvers then work from most specific to least specific. I had an idea for IBM PHD/javacore dumps of using a name resolver for java.lang.Threads from DTFJ instead of the standard resolver using the name field. 
2.Adding methods to IObject is not a compatible change - we could have a new interface, but alternative implementation of IObject would need to implement that method to take advantage of this idea, which doesn't really need the implementation specifics of the IObject. 
3.Why tree view and not table view as well? 
4.How do we save/display the warning text in HTML for saved reports? 
5.@Subject("java.lang.Object") would probably do the same as "*". 


Andrew Johnson 




From:        Kevin Grigorenko <kevin.grigorenko@xxxxxxxxxx> 
To:        mat-dev@xxxxxxxxxxx, 
Date:        25/02/2013 16:16 
Subject:        [mat-dev] Proposal: MAT Warning System 
Sent by:        mat-dev-bounces@xxxxxxxxxxx

---

Proposal for an extensible warning system in MAT:

1. A new extension point named org.eclipse.mat.api.warningResolver. In a similar fashion to nameResolver, a class would be annotated with @Subject and implement an interface IClassSpecificWarningResolver with a method named resolveWarning. This would return an instance of a class WarningResolution which has two items: an enumeration value (Warning would be the only choice initially) and a "details" String.
2. A method is added to IObject: List<WarningResolution> getClassSpecificWarnings();
3. In a tree view, in the same way that any nameResolvers are called for each row, any warningResolvers would be called for each row. If a warningResolver returns a Warning result, the row is color coded. If there are multiple warning resolvers, the highest enumeration value is the one chosen. By default, the color for warning would be red, but there should be a preference to choose the color. Additionally, the context menu is modified to add an item at the top called "Warning Details." When clicked, this raises a dialog with the "details" Strings returned from the warningResolvers previously.
4. In an HTML view, any tree views converted to HTML tables should also have color coded rows, if applicable.
5. There is a special subject @Subject("*") which instructs MAT to call it's resolveWarning method for every class. This could be used by class-independent warning analysis, for example: Warn if an object's retained heap is > x% of total heap size.
6. A set of preferences are added: A checkbox to completely enable or disable the warning system (enabled by default) along with a way to choose the color for a warning.

Let me know what you think. If the proposal is good, I'll start working on a patch and send it to the group for review when ready. Also, this is changing the API (adding an extension point and adding a method to IObject) -- does that affect versioning, etc.?

Some other thoughts:
1. I thought about making IClassSpecificWarningResolver extend IQuery, and then clicking the warning details context menu item would run an instance of IClassSpecificWarningResolver to show a normal query. The problem is that this would be complicated to deal with if there are two or more warning resolvers for the same class and it would complicate the implementation for extensions.
2. An additional report could exist on the Overview screen under Reports named "Show Any Warnings." This would require finding all warningResolver extensions and passing each instance of the subject classes to check for warnings.
3. There could also be a preference to automatically run the report in #2 when loading a heapdump and show any warnings in a section in the Overview report.4. Options #2 and #3 could of course be very expensive so maybe an extension marks itself somehow as able to check for important warnings in some lightweight way, perhaps through a separate method call instead of passing all object instances to resolveWarning.
4. Options #2 and #3 could of course be very expensive so maybe an extension marks itself somehow as able to check for important warnings in some lightweight way, perhaps through a separate method call instead of passing all object instances to resolveWarning.

--
Kevin Grigorenko
IBM WAS SWAT Team
kevin.grigorenko@xxxxxxxxxx
Blog: https://www.ibm.com/developerworks/mydeveloperworks/blogs/kevgrig/?lang=en_______________________________________________
mat-dev mailing list
mat-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/mat-dev


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU_______________________________________________
mat-dev mailing list
mat-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/mat-dev
_______________________________________________
mat-dev mailing list
mat-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/mat-dev