JOL Hotspot SA Support and Compressed References Implementation

[Aleksey Shipilev]

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:

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

 * I think class names may be shorter: HS_SA_* is a reasonable shortcut.

 * 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.

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.

 * 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.

 * 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

* 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.

 * HotspotSACompressedReferencesResult -- why setters even exist? Fields
should be final. toString() is the useless auto-generated stub?

 * 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
anyhow.

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


VMSupport:

 * Bug: formatAddressAsHexByAddressSize(long address) argument is unused

 * Naming: OOP_SIZE vs KLASS_PTR_SIZE. Should be 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?

Thanks,
-Aleksey