JOL Hotspot SA Support and Compressed References Implementation

[Serkan ÖZAL]

Inline.
> Hi again,
>
> On 01/06/2015 07:27 PM, Serkan ÖZAL wrote:
>   
>> I have just reformated the code as 4-space instead of tabs and added the
>> second version of the updated patch to this mail.
>>     
>
> I think the approach is very sound, but the implementation seems to be
> overblown without a compelling reason to do so. Pro-tip: do not try to
> write the broadly generic code until you see the need for it.
>
> General things:
>   
Thanks, I will fix all of the issues as soon as possible and will send 
new patch.

>   * Try to build the project with JDK 8. Javadoc will complain a lot
> about the misuse of @link tags.
>   
OK. Fixed

>  * I think class names may be shorter: HS_SA_* is a reasonable shortcut.
>   
OK. Classs names has been updated as HS_SA_*

>  * I think the excess genericity is redundant at this point, and
> constitutes over-engineering. Inheritance, covariance, and some
> typechecks should work all right in these scenarios. Especially given
> that you can't enforce type safety across JVMs anyway.
>   
Even though, I generally prefer generic types :), I removed all of them 
from patch. So there is no generic type as you suggested.

> HotspotSA*:
>
>  * Is there a way to try and attach without super-user privileges first,
> and the fall-back to "sudo" on failure? Asking for user password
> interactively is asking for trouble, especially for library users.
> Therefore, we are better off trying to attach automatically, and ask for
> the password if that fails *and* some user property is set.
>   
OK. I will work on getting password interactively. In addition, what 
about getting password also as VM argument ? WDYT ?

>  * Why do you need the additional AgentConnectionHandlerThread to deal
> with SA? Is this only to handle the timeouts? If so, you might as well
> ditch the additional thread, and timeout on Process.waitFor.
>   
Right, I have thought again and understood that 
"AgentConnectionHandlerThread" is not necessary for only timeout handling.
Just removed it and wait process to terminate with "waitFor" method.

>  * The tidbit with forking Java processes is that you need to consume
> both stdout/stderr, otherwise you may block indefinitely. This probably
> means you need to start Process, attach stdout+stderr scrubbers, waitFor
> process, and then process the results accumulated by scrubbers. See e.g.
> how JMH does it:
>  http://hg.openjdk.java.net/code-tools/jmh/file/2053f4bab01b/jmh-core/src/main/java/org/openjdk/jmh/runner/Runner.java#l692
>   
Right. Also error stream has been drained if there is something to read 
as your suggested order.

> * HotspotServiceabilityAgentRequest and
> HotspotServiceabilityAgentResponse have unused constructors? I think the
> lifecycle for at least the response is wrong. Instead of having
> setResult/setError, use the constructors, and make the fields final.
>   
OK. Setters have been removed and fields are now final.

>  * HotspotSACompressedReferencesResult -- why setters even exist? Fields
> should be final. toString() is the useless auto-generated stub?
>   
OK. Setters have been removed, fields are now final and "toString" 
method has been removed.

>  * What is the actual use for HotspotSAContext? I don't see it is used
> anywhere, except for a few declarations. It should be immutable as well
>   
"HotspotSAContext" is wrapper class to hold common necessary objects for 
Hotspot Serviceability Agent API Usage.
It is passed to HS_SA_Processor's "process" method as parameter and 
designed for no-change on signature of this method since other objects 
may be added to context at the next versions.

OK. I made it immutable.

> anyhow.
>
>  * osName.indexOf(...) >= 0 should be osName.contains(...)
>   
OK. Refactored.

>
> VMSupport:
>
>  * Bug: formatAddressAsHexByAddressSize(long address) argument is unused
>   
OK. Good catch. Fixed as using "address" argument.

>  * Naming: OOP_SIZE vs KLASS_PTR_SIZE. Should be KLASS_OOP_SIZE?
>   
"KLASS_PTR" term comes from Hotspot SA codes, not my choice. So should I 
update as "KLASS_OOP_SIZE" ?

>  * I fail to understand these comments: " // For backward compatibility,
> OOP compressed reference mode can be used as default compressed
> reference mode". Maybe you should provide a paragraph explaining what's
> going on?
>   
I mean that, there is two different compressed references (OOP and 
Klass) information since Java 8. For Java 6 and 7, there is only one 
(OOP) and this one is used both OOP and Klass.
So I assume compressed-oop information as compressed-reference 
information for users of  "USE_COMPRESSED_REFS" and 
"COMPRESSED_REF_SHIFT" properties.
I hope this time it is more clear :)

> Thanks,
> -Aleksey
>
>
>   
Regards.

--

Serkan ÖZAL