review (XS) for 6956958: assert(is_clean() || is_call_to_compiled() || is_call_to_interpreted() || is_optimized() || is_megam

[Tom Rodriguez]

Oops, left that out when I peeled it out of another workspace.  It's there now.

tom

On Jun 18, 2010, at 1:45 PM, Vladimir Kozlov wrote:

> Static init still has type CodeBlob*:
> 
>  29 CodeBlob* NMethodSweeper::_current = NULL;   // Current nmethod
> 
> Otherwise looks good.
> 
> Vladimir
> 
> Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6956958
>> 6956958: assert(is_clean() || is_call_to_compiled() || is_call_to_interpreted() || is_optimized() || is_megam
>> Reviewed-by:
>> When the sweeper scans the code cache it divides it up into
>> NMethodSweepFraction chunks and processes one chunk per sweep.  Some
>> changes late in the review of the concurrent sweeper changed the
>> interation to skip by nmethods instead of CodeBlobs and during a
>> change late in the review some code which was only supposed to make
>> sure we had stopped on an nmethod was changed into skipping to the
>> next nmethod.  This meant that sweeper skipped processing the last
>> nmethod of each chunk.  Each nmethod is processed multiple times so
>> most of the time this was fine but occasionally the stars aligned and
>> the same nmethod was skipped every time.  This could leave it pointing
>> at a dead nmethod which could cause failures of the sort we were
>> seeing.  Logging code I added for a debug build showed precisely these
>> events occurring when the crash happened.  The fix is simply to remove
>> the extra call to next.
>> While investigating this I found several other issues that are
>> potential problems but they all appeared pretty benign.  I'm going to
>> fix those issues another another bug since they are somewhat larger
>> changes.
>> src/share/vm/runtime/sweeper.hpp
>> src/share/vm/runtime/sweeper.cpp