RFR (L) JDK-8195100: Use a low latency hashtable for SymbolTable

[Gerard Ziemski]

hi Coleen,

Thank you for the review - new webrev at http://cr.openjdk.java.net/~gziemski/8195100_rev2

I like your suggestions and implemented them all, except this one:

>  101 private:
>  102 public:
> 
> 
> Can you remove 101 and indent "public:" by one?   The single indentation is somewhat the convention.

Which file is this in? I don’t see it.

I havel also changed all (NULL == x) into (x == NULL), since there is no universal agreement on the former being preferred, and which I personally dislike, but used only because I thought we wanted it that way.

Lastly, I made the ShortLivedSymbolCleanup.java test more robust (it actually was not doing its job in rev1)

Running Mach5 hs-tier1,2,3,4,5,6 right now…

p.s. Sorry about frames (or sdiffs) not working properly in the webrev - does anyone know what’s wrong?


cheers

> On Aug 1, 2018, at 8:23 AM, coleen.phillimore at oracle.com wrote:
> 
> 
> http://cr.openjdk.java.net/~gziemski/8195100_rev1/src/hotspot/share/classfile/symbolTable.cpp.html
> 
> For some reason, frames in webrev doesn't work for SymbolTable.cpp.
> 
>   44 #define PREF_AVG_LIST_LEN           4
> 
> 
> Maybe this should be something like 8, because there can be outlier long bucket lists for any random hashcode.
> 
>   50 #define REHASH_LEN                  32
> 
> 
> Maybe this should be 100.   This was the old heuristic for rehashing.
> 
> // Check to see if the hashtable is unbalanced.  The caller set a flag to
> // rehash at the next safepoint.  If this bucket is 60 times greater than the
> // expected average bucket length, it's an unbalanced hashtable.
> // This is somewhat an arbitrary heuristic but if one bucket gets to
> // rehash_count which is currently 100, there's probably something wrong.
> 
> It looks like the StringTable sizes are smaller.  Maybe just leave these, and we can experiment with shared values for the StringTable and SymbolTable at some later time.
> 
>  101 private:
>  102 public:
> 
> 
> Can you remove 101 and indent "public:" by one?   The single indentation is somewhat the convention.
> 
>  138 _needs_rehashing(false), _items(0), _uncleaned_items(0) {
> 
> 
> indent this line too.
> 
>  177   Atomic::add((size_t)-1, &(SymbolTable::the_table()->_items));
> 
> 
> Should this be Atomic::sub, and is the cast necessary?
> 
>  377     *is_dead = (sym->refcount() == 0);
>  378     if (*is_dead) {
>  379       return false;
>  380     }
> 
> 
> Did we decide that it was unnecessary to pre-check the refcount, since the condition is rare?
> 
>  568   Symbol* sym = SymbolTable::lookup_only((char*)name, len, hash);
> 
> 
> I don't think you need this cast to char*.
> 
>  887   {
> 
> 
> I don't see why you have this scope.
> 
> Except for these cosmetic comments, everything else looks fine.  This is great work, Gerard!
> 
> Thanks,
> Coleen
> 
> 
> On 7/30/18 12:19 PM, Gerard Ziemski wrote:
>> Please review this Enhancement, which uses the new concurrent hash table for SymbolTable.
>> 
>> This is an effort similar to the one behind JDK-8195097 "Make it possible to process StringTable outside safepoint” from a while ago.
>> 
>> The main expected goal here is to eliminate safepoint pauses needed to cleanup the table. This goal was achieved by using “Service Thread” to do the cleaning. Checking whether we need to clean is performed on “VM Thread”, after class unloading (we check the entire table). We also check the bucket into which we happen to be inserting a new item.
>> 
>> A few things to note:
>> 
>> - The SymbolTable implementation follows closely that of StringTable - we might be able to factor out common code later
>> 
>> - There are a few small, but statistically significant, regressions in startup benchmarks (around 2-5%) that will be addressed later, tracked by JDK-8208142
>> 
>> - There is an outstanding question about whether we can safely walk the table during a safepoint using do_scan, but without locking, tracked by JDK-8208462
>> 
>> - There is a cleanup opportunity presented now to remove rehashable hash table, tracked by JDK-8208519 
>> 
>> - There is a new test that validates that we free dead entries when we insert new symbols (using short lived symbols via Class.forName() API)
>> 
>> 
>> Tested using Mach5 hs-tier1,2,3,4,5 (final test running right now…)
>> 
>> Webrev: 
>> http://cr.openjdk.java.net/~gziemski/8195100_rev1/
>> 
>> 
>> Bug: 
>> https://bugs.openjdk.java.net/browse/JDK-8195100
>> 
>> 
>> 
>> Cheers
>> 
>> 
>