Re: [RFC] Enhanced Garbage Collection Probe Points

[Lukas Berk <lberk at redhat dot com>]

Hey Jon,

Thanks for the comments, I've resolved them or explained them inline.
* Jon VanAlten <jvanalte@redhat.com> [2012-08-24 15:59]:
> 
[...]
> We have a hotspot.stp, a hotspot_jni.stp; maybe this one should be
> hotspot_gc.stp?
Done, its now included as hotspot_gc.stp.in
> 
> Adding this fluff to the existing systemtap.patch, may not be the
> best way.  As I understand it, Mark is trying to get the existing
> stuff upstream.  Combining this into same IcedTea patch may make
> it more difficult if/when this upstreaming is eventually successful
> and needs to be removed from IcedTea.  I'd suggest introducing a
> separate patch to the icedtea sources.
Makes sense to me, I've adjusted the autoconf files accordingly to
include and apply patches/systemtap_gc.patch if icedtea is configure
with --enable-systemtap.
> 
> + * @size: word size to be cleaned
> 
> (in several places)
> This wording (pun not intended) is not entirely clear.  Playing the
> ignorant reader, I am not sure if this means the wordsize that the
> jvm uses, or the total space, in words, that will be collected on
> this gc run.  Can this be clarified?
I've changed this to: "Word size of the object to be collected."
> 
> + * @is_full: If TRUE, attempt a full collection of the generation
> 
> And if not TRUE?  (again playing the ignorant reader).
if its false then perform a scavenge, I've adjusted the docs accordingly
> 
> + * Description: This marks the end of a parallel collection of a new 
> + * generation.
> + * This is different than a gc_collect_parallel generation due to it being
> + * specifically defined as a parNewGeneration
> 
> The 'clarification' sentence reads to me like "Y is not X because it
> is Y".  I'd consider leaving it off, or providing a better explanation.
I see what you're saying.  I've removed it.
> 
> + * Description: The start of a newly defined generation collection
> 
> The previous probes' documentation says "This marks the...", I'd love
> to see consistency in the wording.
I've changed this to be consistent throughout. Also, I've made sure that
all capitalization and punctuation is correct/consistent.
> 
> + * Description: This is the start of a collection of a tenured generation
> + * (a geneartion that has survived multiple garbage collections and is 
> 
> Here too, also typo "generation"
Fixed.
> 
> + * probe - gc_collect_parallel_scavenge
> + * 
> + * @name: gc_collect_parallel_scavenge
> + * @address: address of object being collected
> + * @cause: cause of the collection
> + * 
> + * Description: This is a parallel collection, where the jvm process don't
> + * have to halt while the gc is being completed
> 
> I want to make sure I understand this.  If I read this correctly, this
> probe fires many times between a begin and end of parallel gc run, for
> each object that is scavenged?  If I do understand this, then saying
> "this is a parallel collection" I think is the wrong wording.  This
> is not a collection, but an event that is part of a collection run.  If
> I misunderstand, please clarify :)
No, a scavenge is just a miniature collection, not a process within a
collection.  The mechanics are the same, just not on the entire object
space/heap.  That being said I've s/This is a parallel collection/This
is a parallel scavenge/ to be specific.
> 
> + * Description: A delete statement of an obeject
> 
> Typo "obeject"
Fixed.
> 
> Otherwise, I don't really have any concerns with this.  I hope though
> that Mark (who introduced the systemtap support for the existing
> probes) could also take a look for sanity.  I've added him to CC
> directly, even though he probably also gets this through the list.
Keeping him on the CC
> 
> on the benchmarking results and the interest in my patch, 
[...]
> 
> I think there's more to look at in terms of performance.  However,
> despite what the numbers here show, from my understanding (Mark,
> please chime in here!) the existence of these probes in the source
> should in theory not affect runtime performance.  Otherwise, I might
You are correct, from a performance perspective, adding probe points is
essentially within the realm of noise.  I just wanted to show that
actually using them won't cause significant performance hits if anybody
had any reservations.
> be asking to see data for --enable-systemtap w/o this patch as well
> to compare really what this patch adds.  Anyhow, unless I am quite
> wrong about this, I don't think there is any performance concern
> for adding these probes, only for when they are used.  So, consider
> my next paragraph not for this patch, but more generally to discuss
> performance impact of using these probes.
> 
> These numbers do seem to raise some questions, though.  Why do we
> see such variation between the runs?  I'd encourage you to explore
> this further.  Another aspect to consider: looking at run time of
The variations can most likely be attributed to the fact I was
running this on a vm on my laptop and had other (non trivial)
processes running on the host.
> benchmark will show the impact aggregated over entire java process,
> but it would be interesting to see the impact relative to time
> actually spent in gc.  I'd be concerned if simply printing this
> probestr at beginning and end of gc made the gc run last 10% longer,
> even if it looks relatively minor aggregated over full java process
> run time.  I'm not sure how you'd go about trying to measure this,
> however.
I can look into running scripts that would simply increment a variable
(ie gc_hit++) every time a probe is hit instead of logging the probestr,
also I could run the same tests and start a timer when hotspot.gc_begin
is hit and ends when a hotspot.gc_end is hit. (those probes are already
there).
> 
> > what else
> > needs to be done to get this patch applied to icedtea?
> > 
> 
> Well, I am basically OK with this aside from the things mentioned
> above.  I guess remaining still is the question of testing these.  I
> wouldn't block this based on lacking tests (the previous probes went
> untested for ages before I added tests), but I also don't want this
> to go in without a plan to eventually add tests; when we did add
> tests for the other probes, we actually found regressions and even
> things that had never properly worked iirc.
> 
> Have you given any thought to how you might add tests for these
> probes to the existing set of probe tests?
Ideally I'd have some java test programs that would be able to allocate
enough objects to trigger collections.  As mentioned earlier, different
gc algorithms can be specified with jvm options, but what would be
tricky is controlling the gc invocation via jvm options in a reliable
enough manner that we can reasonably expect scavenges to occur.  I would
have to experiment with that to see if its possible, if anybody else
knows how to easily do that please let me know.

Is this ok to commit?

Cheers,

Lukas

Attachment: icedtea.patch
Description: Text document

Attachment: pgp00000.pgp
Description: PGP signature