Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 365533

Summary:	Move validation of indices into the core of MAT
Product:	[Tools] MAT	Reporter:	Erik Brangs <erik.brangs>
Component:	Core	Assignee:	Andrew Johnson <andrew_johnson>
Status:	RESOLVED FIXED	QA Contact:
Severity:	enhancement
Priority:	P3
Version:	unspecified
Target Milestone:	---
Hardware:	All
OS:	All
Whiteboard:

Description Erik Brangs

2011-12-04 17:17:52 EST

MAT has constraints on the indices that are built by the heap dump plugins (e.g. DTFJ and HPROF). Each plugin has to verify these constraints independently.

It would be nice if MAT could verify these constraints as suggested by Andrew's post in the thread http://www.eclipse.org/forums/index.php?t=msg&th=163200&start=0 on the old MAT forum.

If there are plugin-specific constraints MAT could also provide some hooks for them.

Comment 1 Andrew Johnson

2011-12-05 12:26:02 EST

This makes sense - I'll have a go at moving it to SnapshotFactoryImpl.java

Comment 2 Andrew Johnson

2011-12-06 05:02:19 EST

I'm moving the validation across and adding some checks for outbound references and thread roots.

Comment 3 Krum Tsvetkov

2011-12-06 08:14:53 EST

I also like the idea to move the validation checks to core.
However, I wonder if we should execute the checks with every heap dump.
The validation loops over all objects and reads from different indexes. With bigger heap dumps this will cause a noticeable performance degradation (I think).
On the other hand, what the validation checks do is roughly said "check if the internal logic of the parser was correct". 
This check is great for developers working on a parser implementation, but I see no profit for the end user, rather the opposite - impact on the performance (just correct me if I am making the wrong assumptions).
Therefore I would suggest that:
 - the checks are performed in the core, so that any parser implementation can profit from them
 - the checks are by default turned off, and there is a possibility to turn them on (for example when working on the parser)
 - end users may also turn on the checks in case of problems

What do you think?
 
Besides this, I am not quite sure if the SnapshotFactory class is the proper place for the validation... Shall we move the validation logic into a class on it's own (used by SnapshotFactory)?

Comment 4 Andrew Johnson

2011-12-06 09:37:35 EST

Moving to a new class in the same project should be straightforward.
We could do it conditionally based on a plugin debug flag, as used in the o.e.mat.report and o.e.mat.dtfj plugins.

Comment 5 Erik Brangs

2011-12-06 16:50:01 EST

A debug flag would probably be sufficient for developers. Another possibility would be to use assertions.

If users should be able to turn on the index validation, an option in the MAT preference window could be used.

Comment 6 Andrew Johnson

2012-01-11 08:53:31 EST

The validation is now conditional on the plugin debug flag:
http://wiki.eclipse.org/MemoryAnalyzer/Adding_a_new_heap_dump_format_to_the_Memory_Analyzer#IIndexBuilder
The validation is still in the SnapshotFactoryImpl class. We could move it, but this is an internal class so I did not bother.