review request (L) - 6423256 GC stacks

[John Coomes]

Scott Marlow (scott.marlow.opensource at gmail.com) wrote:
> John,
> 
> A few comments.  First, thank you for bringing this change out for review.

Hi Scott,

Many thanks for taking a look.

> 1.  If anyone on the list happens to have any performance jdk tests 
> handy, it would be interesting to see before/after results of applying 
> this change (so that any regressions can be addressed and improvements 
> shared :-).  If there is no noticeable impact, that is worth sharing 
> too.  I no longer have access to the torture test that I once had (a 
> four node cluster that reproduced a production environment that 
> experienced bug # 6423256).  If no one has performance tests to run, 
> that shouldn't block applying this patch (would just be nice to have).

I didn't mention it, but I've run our standard performance benchmarks
(specjbb2005, specjvm98, etc.) on a few architectures & operating
systems.  It's standard procedure for most changes, so many of us
who've been doing it for years don't bother to mention it anymore.  As
expected there's no statistically significant change in either overall
performance or GC performance.  This code isn't on a super hot path,
and the common case operations are basically identical to the
GrowableArray code it replaces.

> 2.  I like the idea expressed in stack.hpp about possibly caching 
> segments in thread local storage.  At least, that is the optimization 
> that I assume is meant by saving the thread id.  Or are you just talking 
> about using it for debugging purposes (for tracking which thread 
> allocated each segment)?  I have worked on heap sub-allocators for other 
> virtual machines but haven't learned about the Java native allocators 
> yet.  I would think that having a per thread segment cache, could have 
> some impact on the native memory pool (potentially causing fragmentation 
> depending on how many threads are in use).  Although, the pressure on 
> the native heap should be much lower than what growableArray must of 
> done to the heap.  This isn't a serious concern, since its just a 
> comment but thought it was worth raising.

It's a good question.  The comment refers to the fact that the alloc()
and free() methods for a ResourceStack need to use the HotSpot Thread
object, so caching it instead of looking it up each time would save a
a modest number of cycles each time you alloc/free a segment.  Right
now there are no uses of ResourceStack, so it's mostly just a note for
the future.

> 3.  What is the meaning of "const size_t limit = size_t(-1) - (seg_size 
> - 1)" in stack.inline.hpp (adjust_max_size)?  It looks like you are 
> trying to determine the largest possible size but I wanted to make sure 
> as I'm not familiar with how "size_t(-1)" works.  Could there be 
> problems with that expression on some of the exotic machine 
> architectures that Java is running on (e.g. 1s complement cpu running 
> j2me).

Right, I'm using size_t(-1) to get the largest size_t; it's an
alternate syntax for the cast (size_t)-1 that makes it look like
you're "constructing" a size_t.  And it's not precise on a
1s-complement box--it would be one less than the max size_t.  This
code would still work, but it's not very clean.  Turns out HotSpot has
a global constant max_uintx; I'll use that instead.

Thanks again!

-John

> On 09/21/2009 01:09 PM, John Coomes wrote:
> > I'd appreciate reviews of
> >
> > 	6423256: GC stacks should use a better data structure
> >
> > 	http://cr.openjdk.java.net/~jcoomes/6423256-gc-stacks/
> >
> > See the comments at the top of the webrev and in file stack.hpp for
> > some hints/details.
> >
> > -John
> >
> >
> >    
>