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

Mikael Gerdin mikael.gerdin at oracle.com
Tue Jan 28 14:00:19 UTC 2014

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::unlinkoroopsdo() methods, > but only separate StringTable/SymbolTable::unlink() and > StringTable/SymbolTable::oopsdo() methods. > > - 7u60 allows sharing of strings using CDS, making minor changes > necessary. In particular, in StringTable::bucketsunlink(), 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->isshared() && !usealternatehashcode()) { 825 break; 826 } 827 assert(entry->literal() != NULL, "just checking"); 828 if (entry->isshared() || isalive->doobjectb(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->isshared(), "CDS not used for the StringTable"); 836 837 if (isalive->doobjectb(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()

= initialsymboltablesize, 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

More information about the hotspot-gc-dev mailing list