RFR (M): (7u60): Backport of 8027476: Improve performance of Stringtable unlink, 8027455: Improve symbol table scan times during gc pauses

[Mikael Gerdin]

Thomas,

On Tuesday 28 January 2014 11.57.33 Thomas Schatzl wrote:
> Hi,
> 
>   I got the link for the new webrev wrong, taking two from the original
> patch. Sorry.
> 
> On Tue, 2014-01-28 at 10:51 +0100, Thomas Schatzl wrote:
> > Hi all,
> > 
> >   could I have reviews for the following backport of the symbol table/
> > 
> > string table parallelization change?
> > 
> > It implements parallelization of string table and symbol table scan
> > during G1 full GC and remark for 7u60.
> > 
> > There is some difference in the patches due to how 7u60 managing the
> > iteration over the string table:
> > 
> > - 7u60 has no StringTable/StringTable::unlink_or_oops_do() methods,
> > but only separate StringTable/SymbolTable::unlink() and
> > StringTable/SymbolTable::oops_do() methods.
> > 
> > - 7u60 allows sharing of strings using CDS, making minor changes
> > necessary.
> 
> In particular, in StringTable::buckets_unlink(), the code that considers
> shared strings need to be kept alive, i.e. in
> http://cr.openjdk.java.net/~tschatzl/8027476.7u60/webrev/src/share/vm/classf
> ile/symbolTable.cpp.frames.html , in the right (new) version the following
> code.
> 
>  820       // Shared entries are normally at the end of the bucket and
> if we run into
>  821       // a shared entry, then there is nothing more to remove. However,
> if we 822       // have rehashed the table, then the shared entries are no
> longer at the 823       // end of the bucket.
>  824       if (entry->is_shared() && !use_alternate_hashcode()) {
>  825         break;
>  826       }
>  827       assert(entry->literal() != NULL, "just checking");
>  828       if (entry->is_shared() ||
> is_alive->do_object_b(entry->literal())) {
> 
> In 8u20/9 the corresponding code is just:
> 
> http://cr.openjdk.java.net/~tschatzl/8027476/webrev.1/src/share/vm/classfile
> /symbolTable.cpp.frames.html , again, right frame:
> 
>  835       assert(!entry->is_shared(), "CDS not used for the
> StringTable");
>  836
>  837       if (is_alive->do_object_b(entry->literal())) {
> 
> Automatic merge got confused and simply did not take this difference
> into account. I hope this clears this recent, rather cryptic comment up.
> 
> > These differences are contained in symbolTable.?pp, the rest is
> > verbatim backport iirc.
> > 
> > New webrev:
> > http://cr.openjdk.java.net/~tschatzl/8027476/webrev
> 
> Correct webrev is at:
> 
> http://cr.openjdk.java.net/~tschatzl/8027476.7u60/webrev

symbolTable.cpp:

 103       Symbol* s = entry->literal();
 104       memory_total += s->object_size(); << should be (*memory_total) += 
 105       (*processed)++;

g1ColletedHeap.cpp:

5237     guarantee(StringTable::parallel_claimed_index() >= 
_initial_string_table_size,
5238               err_msg("claim value "INT32_FORMAT" after unlink less than 
initial string table size "INT32_FORMAT,
5239                       StringTable::parallel_claimed_index(), 
_initial_string_table_size));
5240     guarantee(!_process_strings || SymbolTable::parallel_claimed_index() 
>= _initial_symbol_table_size,
5241               err_msg("claim value "INT32_FORMAT" after unlink less than 
initial symbol table size "INT32_FORMAT,
5242                       SymbolTable::parallel_claimed_index(), 
_initial_symbol_table_size));
5243 

You've removed the !_process_strings from the guarantee compared to 9.
Also, I noticed that the 9 versions has !_process_strings in both guarantees 
although I think the second one should be !_process_symbols.

/Mikael

> 
> > Old Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8027476/webrev.1
> > 
> > CRs:
> > https://bugs.openjdk.java.net/browse/JDK-8027455
> > https://bugs.openjdk.java.net/browse/JDK-8027476
> > 
> > Testing:
> > jtreg test case, jprt, FMW apps
> > 
> > Thanks,
> > 
> >   Thomas
> 
> Sorry,
>   Thomas