More reliable compressed-oops patch for JOL and some simple improvements [Patch Linked]

[serkan özal]

Hi Aleksey,
Thanks for your comments. 
My comments are inline.
> * I think the code is in a dire need of "Implementation notes" section
> explaining what it tries to do. Previous code was also scarce on that,
> but with this change, it is really over the top.

Right, I can write an implementation details comments at last to describe what we did for compressed-oops.


> * Are we sure the magic offsets in HotspotJvmClassAddressFinder are
> stable across JDKs? I would not be that sure, but let's talk about that
> later.

Currenly, I got out these magic offsets from native codes of JDK. In my opinion, to be sure about them,     * Test as much as all published JDK binaries :) (for JDK 7, such as jdk1.7_u17, jdk1.7_u21, etc ...)    * Looking commit logs of these files if ltheir ayout is changed or not.
What do you think about how we can be sure for these magic offsets?


> * Do we need to have the AbstractHotspotJvmClassAddressFinder
> hierarchy? It seems you can just turn the Abstract* class into the
> concrete one, and accept the offsets there.
Yeah, we can use just concrete "HotspotJvmClassAddressFinder" by giving offsets as argument.



> * JvmInfo and JavaVersionInfo enums feel redundant. Would you like to
> just make JAVA*.equals() calls in the getters like isJava_6()?

Ok, will replace enums with "JAVA*.equals()" like calls.



> * Please use shorter identifiers. Identifiers like
> CLASS_DEFINITION_POINTER_OFFSET_IN_CLASS_INSTANCE_64_BIT_WITHOUT_COMPRESSED_REF_FOR_JAVA_8
> are actually hard to read. CLASSDEF_CI_OFFSET_64_JAVA8 seems better.
> This applies to field/local names as well.

Right, they are too long :) will trim them.



> * We use 4 spaces as the indentation units, not tabs. Please also
> auto-format the source, you have the indentation gone wrong a few times.

sure, will format with spaces instead of tabs.


> * Do we actually need these two guys?
> private static final byte ADDRESS_SIZE_4_BYTE = 4;
> private static final byte ADDRESS_SIZE_8_BYTE = 8;

yes, can use just 4 or 8 instead of constant



> * "////////////////////////" is not a good delimiter in our codebase.
> Use "/* ------------------------- */"?
 Ok



Regards.
--
Serkan ÖZAL
> Date: Tue, 9 Sep 2014 19:48:43 +0400
> From: aleksey.shipilev at oracle.com
> To: serkanozal86 at hotmail.com; jol-dev at openjdk.java.net
> Subject: Re: More reliable compressed-oops patch for JOL and some simple improvements [Patch Linked]
> 
> Hi Serkan,
> 
> On 09/08/2014 10:20 PM, serkan özal wrote:
> > Patch Link: https://raw.githubusercontent.com/serkan-ozal/dropbox/master/compressed-oops.patch 		 	   		  
> 
> Thanks for doing this! I think we would need a few rounds of code
> reviews to see what is going on there.
> 
> Comments:
> 
>  * I think the code is in a dire need of "Implementation notes" section
> explaining what it tries to do. Previous code was also scarce on that,
> but with this change, it is really over the top.
> 
>  * Are we sure the magic offsets in HotspotJvmClassAddressFinder are
> stable across JDKs? I would not be that sure, but let's talk about that
> later.
> 
>  * Do we need to have the AbstractHotspotJvmClassAddressFinder
> hierarchy? It seems you can just turn the Abstract* class into the
> concrete one, and accept the offsets there.
> 
>  * JvmInfo and JavaVersionInfo enums feel redundant. Would you like to
> just make JAVA*.equals() calls in the getters like isJava_6()?
> 
>  * Please use shorter identifiers. Identifiers like
> CLASS_DEFINITION_POINTER_OFFSET_IN_CLASS_INSTANCE_64_BIT_WITHOUT_COMPRESSED_REF_FOR_JAVA_8
> are actually hard to read. CLASSDEF_CI_OFFSET_64_JAVA8 seems better.
> This applies to field/local names as well.
> 
>  * We use 4 spaces as the indentation units, not tabs. Please also
> auto-format the source, you have the indentation gone wrong a few times.
> 
>  * Do we actually need these two guys?
>         private static final byte ADDRESS_SIZE_4_BYTE = 4;
>         private static final byte ADDRESS_SIZE_8_BYTE = 8;
> 
>  * "////////////////////////" is not a good delimiter in our codebase.
> Use "/* ------------------------- */"?
> 
> Thanks,
> -Aleksey.
>