Bugzilla – Full Text Bug Listing
| Summary: | Add Support for UseCompressedOops | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] MAT | Reporter: | Chris D. Johnson <cdj06> | ||||
| Component: | Core | Assignee: | Krum Tsvetkov <krum.tsvetkov> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | andrew_johnson, mauromol | ||||
| Version: | unspecified | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Chris D. Johnson
If someone wanted to fix this then some steps to do so could be: 1.generate a dump with -XX:+UseCompresssedOops, preferably using org.eclipse.mat.tests.CreateSampleDump 2.generate a dump without -XX:+UseCompresssedOops, preferably using org.eclipse.mat.tests.CreateSampleDump 3.get a list of some expected object sizes for 1 and 2 4.write some test cases cheching the sizes, including for simple objects, array objects, class objects 5.See if there is a way of distinguishing dumps from case 1 and 2, otherwise have a preference and/or command line option 6.examine HprofParserHandlerImpl.calculateInstanceSize(), calculateClassSize(), sizeOf() 7.change the size of reference field from 8 to 4 for the -XX:+UseCompresssedOops 8.change the size of the object header as appropriate e.g. is the header 2*4 bytes? I've added some test dumps and corresponding jmap output /org.eclipse.mat.tests/dumps/sun_jdk6_30_x64_compressedOops.hprof /org.eclipse.mat.tests/dumps/sun_jdk6_30_x64_compressedOops.notes.txt /org.eclipse.mat.tests/dumps/sun_jdk6_30_x64_nocompressedOops.hprof /org.eclipse.mat.tests/dumps/sun_jdk6_30_x64_nocompressedOops.notes.txt Andrew, thanks for the hints and the dumps. I'll have a look at this. In the first place I'll add some coding to calculate sizes with compressed oops based on a property specified by the user. I know it is not perfect, but it should be better that having no possibility at all. I think I found the proper formula to do the calculation. At least I managed to get (still just locally) the sizes I compute to match the sizes from the compressed oops histogram. I'll test a bit more and polish what I have and will then check it in. I've got some code too, so let's merge our ideas. Created attachment 210973 [details]
enable compressed oops (first draft)
Andrew, I attached as a patch what I have:
- removed some content from the histograms (headers and totals line)
- added two tests - comparing the histograms with our computations for compressed and non-compressed oops
- added some code to read a property
- adapted the computation of the size
This still needs cleanup, e.g. at the moment I check if the property is set on every call to sizeOf(), which could be optimized.
But at least you can see if this goes the same direction as your changes
The tests pass OK - this is the good news :)
I had some similar code for the class size calculation, plus some changes for array size calculations. Pass1Parser - detect compressedOops by keeping a start address for an object array and an end address assuming uncompressed pointers, then if the next object array start falls in that range and id size = 8 then assume compressedOops and register the fact by calling into HprofParserHandlerImpl HProfParserHandler - have fields for compressed reference size, uncompressed pointer size, object alignment - add methods for array size calculation long getObjectArrayHeapSize(ClassImpl arrayType, int size) long getPrimitiveArrayHeapSize(byte elementType, int size) - calculate object sizes similarly to Krum's patch Pass2Parser - call IHprofParserHandler to do array size calculations The test case doesn't test the array sizes. This would be useful, but is harder. Perhaps it makes sense comparing the total shallow size if the number of objects in the dump and histogram are the same. The autodetection of useCompressedOops might be good enough so we do not need an system property. The alignment is 8 bytes in all modes, except perhaps on some VM implementations which allow compressedOops with heaps >32GB. We may be able to calculate it from object addresses. hprof dumps generated by the hprof agent are a bit different. I've seen 32-bit ids generated from a 64-bit VM, and they are just ids, not addresses, so we can't tell much about the running system. That might be a case where a user needs to configure compressed,uncompressed and alignment sizes. This sounds very good. Hiow shall we proceed: 1) Shall I then check-in my changes first, i.e. the new tests + some minimal implementation using property just to have the test green Then you can add your changes, i.e. better implementation of the sizeOf method + auto-detecting of compressed oops? or 2) you chek in first and then do I :) Krum, I've added my changes to the parser code. Please could you add your test case changes, then I can add the array size tests. Do we need allow the sizes and alignments to be set manually? E.g. for dumps from 64-bit VMs generated by the hprof agent, which just uses 32-bit object IDs not addresses, the code at the moment will calculate sizes as if it were from a 32-bit VM. I added now two small changes, so that the histograms for the tests are parsed properly.
Your changes are in and the tests (no array size tests) are green.
>> Do we need allow the sizes and alignments to be set manually?
I think so. May be we should add as a start one more pair of heap dump and histohram to cover the scenario you mentioned (i.e. running hprof as profiler)?
Further more, I haven't done extensive testing so far. Just ran the tests and tested acquiring locally a dump with and without compressed Oops. I'll try to find some older dumps from 64 bit HP, Solaris, etc... and make sure that the changes didn't affect the sizes there unexpectedly.
And there is may be one more todo - to indicate in the UI in the Heap Dump Details view that the dumps is with compressed oops (or better said that we considered it to be with compressed oops). I've seen a statement in the console/info log, but I think we should make it more transparent. What do you think?
I've added Krum's tests from the patch, and then my array size tests. There was one difference - with the SUN_JDK5_13_32BIT dump the size of the byte arrays was slightly different: Array class byte[] expected total instances size 43,536 but got 43,544 but I don't know if the jmap histogram had exactly the same objects as the dump. See bug 231296 - for other size differences with this dump. Perhaps we should just remove the byte[] entry from the histogram, or comment it out, rather than do special things to the testcase. I would say we should just delete the byte[] line from the JDK 5 histogram. We know that such slight differences are expected, and I don't think it makes sense to spent more time on this. I added two more pairs of dump + histogram for testing - generated with jdk 1.6_31 and using the hprof agent. In both cases (with and withouth compressedOops) there are quite some errors if I activate the tests. Andrew mentioned already before the reason for it - no addresses are used, but IDs instead. Is this a scenario (i.e. dumps generated with the agent) which we would like to support? I guess so. I'll look into this in more details, but comments are welcome. comment#8 > Do we need allow the sizes and alignments to be set manually? E.g. for dumps from 64-bit VMs generated by the hprof agent, which just uses 32-bit object IDs not addresses, the code at the moment will calculate sizes as if it were from a 32-bit VM. Andrew, sorry I got you wrong last time. I thought that we wrongly assume 32 bit ID length for dumps generated with the hprof agent. Now I see that the id size reported in the dump is 32 bit. In this case we don't do wrong assumptions, but believe what the dump says. It may be still useful to add the possibility to manually specify the sizes, but I don't see this as an urgent topic. I'll try first to visualize in the heap dump info the information if we calculated the sizes with compressed oops or not. I added some changes (rev. 1331) to show in the "Heap Dump Details" if compressed pointers are used or not. Thus one can see if the tool recognized the usage of compressed oops. As a readable text for the property I used "Compressed object pointers". Is this fine? It's somewhat long, but I thought it's more understandable than compressedOops. Seeing the status is useful, though for 64-bit IBM dumps it says false, even though the sizes are correct and can allow for compressed object pointers. I can set the $useCompressedOops appropriately in the DTFJ parser. Wouldn't it be better to only display the status if the property is defined, and omit it for dumps parsed with older levels of DTFJ? >> Wouldn't it be better to only display the status if the property is defined, and omit it for dumps parsed with older levels of DTFJ?
you are right, for dumps parsed previously the info shown will be wrong. I'll change the coding to show the status only if it was explicitly set, and not assume missing info => not compressed. You still may change the DTFJ part if you think the info is useful.
I've already made the change to show the status only if it was explicitly set, and for DTFJ, but please feel free to review it. Sorry, I first replied and only then saw your changes :-) I still need to do one modification in the hprof parser. The 1.2.0 release which was recently released contains already the changes to "guess" the use of CompressedOops. Therefore, I think we can close this ticket. In case there are concrete improvement requests or problems with the provided solution we can reopen this one or create new bugs. It would be nice is some of the people on the reporter/CC list can give us feedback if the implemented solution is ok. |