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

[Mikael Gerdin]

Thomas,

Overall I think the change is fine.
I have some comments inline,

On Friday 10 January 2014 10.27.49 Thomas Schatzl wrote:
> Hi all,
> 
>   could I have reviews for the following performance change?
> 
> It implements parallelization of string table and symbol table scan
> during G1 full GC and remark.
> 
> This decreases the respective string table/symbol table scan times
> almost linearly depending on the number of threads used.
> 
> The change is quite straightforward, similar to string table scan during
> initial mark there are new possibly_parallel_unlink() methods for both
> string and symbol table, that are later used in a new taskset.
> 
> Task distribution works as before, using a single claim value for each
> of the tables (reusing the one for the string table) on the hash table
> array.
> 
> The impact on total remark/full gc time is significant, in conjunction
> with JDK-8027454 (RFR coming soon) remark time is halved.
> 
> I decided to merge these two fixes into a single webrev because these
> two things are closely related, and the shared code between the two
> issues is very large, and the actual code to implement one or the other
> is small and easily understood (imo). Splitting would add lots of code
> that would first need to be added and then removed again too.
> 
> I filed two follow-up CRs to use these new methods in the other parallel
> collectors too (JDK-8031473), and another one for fine-tuning
> (JDK-8031472) some parameters.
> 
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8027476/webrev


-  // Now clean up stale oops in StringTable
-  StringTable::unlink(&g1_is_alive);
-  // Clean up unreferenced symbols in symbol table.
-  SymbolTable::unlink();
+  g1h->unlink_string_and_symbol_table(&g1_is_alive);

Why did we call StringTable::unlink here?
AFAICT G1's initial mark calls process_strong_roots with SO_Strings with the 
G1ParScanAndMarkExtRootClosure, which in turn will make all strings which are 
in the StringTable at init mark to be live. Have you ever seen any strings 
being unloaded at this point? ( See 
https://bugs.openjdk.java.net/browse/JDK-8015332 and 
http://openjdk.java.net/jeps/156 )


+  ~G1StringSymbolTableUnlinkTask() {
+    guarantee(!_process_strings || StringTable::parallel_claimed_index() >= 
_initial_string_table_size,
+              err_msg("claim value "SIZE_FORMAT" after unlink less than 
initial string table size "SIZE_FORMAT,
+                      StringTable::parallel_claimed_index(), 
_initial_string_table_size));
+  }

Why do you only check the string table size here and not the symbol table?


/Mikael

> 
> CRs:
> https://bugs.openjdk.java.net/browse/JDK-8027455
> https://bugs.openjdk.java.net/browse/JDK-8027476
> 
> Testing:
> test case, jtreg, FMW apps, specjbb2005, specjbb2013, dacapo, and lots
> of other testing in the recent months
> 
> Thanks,
>   Thomas