RFR(L): 8195097: Make it possible to process StringTable outside safepoint

[Per Liden]

Hi,

On 06/01/2018 11:46 PM, Gerard Ziemski wrote:
> Hi,
> 
> Awesome job Robbin!
> 
> I especially like that we now have self resizing StringTable (though I don’t like the power of 2 size constraint, but understand why that needs to be so).
> 
> I do have some feedback, questions and comments below:
> 
> 
> #1 The initial table size according to this code:
> 
> #define START_SIZE         16
> 
> _current_size = ((size_t)1) << START_SIZE;
> 
> Hence, it is 65536, but in the old code we had:
> 
> const int defaultStringTableSize = NOT_LP64(1009) LP64_ONLY(60013);
> 
> Which on non-64bit architecture was quite a bit smaller. Is it OK for us not to worry about non 64bit architectures now?
> 
> BTW. It will be extremely interesting to see whether we can lower the initial size, now that we can grow the table. Should we file a followup issue, so we don't forget?
> 
> 
> #2 Why we have:
> 
> volatile size_t _items;
> DEFINE_PAD_MINUS_SIZE(1, 64, sizeof(volatile size_t));
> volatile size_t _uncleaned_items;
> DEFINE_PAD_MINUS_SIZE(2, 64, sizeof(volatile size_t));
> 
> and not
> 
> volatile size_t _items;
> DEFINE_PAD_MINUS_SIZE(1, DEFAULT_CACHE_LINE_SIZE, sizeof(volatile size_t));
> volatile size_t _uncleaned_items;
> DEFINE_PAD_MINUS_SIZE(2, DEFAULT_CACHE_LINE_SIZE, sizeof(volatile size_t));
> 
> 
> #3 Extraneous space here, i.e. " name":
> 
> return StringTable::the_table()->do_lookup( name, len, hash);
> 
> 
> #4 Instead of:
> 
> double fact = StringTable::get_load_factor();
> double dead_fact = StringTable::get_dead_factor();
> 
> where "fact" is an actual word on its own, can we consider using full names, ex:
> 
> double load_factor = StringTable::get_load_factor();
> double dead_factor = StringTable::get_dead_factor();
> 
> 
> #5 In "static int literal_size(oop obj)":
> 
> a) Why do we need the "else" clause? Will it ever be taken?
> 
> } else {
>    return obj->size();
> }
> 
> b) Why isn't "java_lang_String::value(obj)->size()" enough in:
> 
> } else if (obj->klass() == SystemDictionary::String_klass()) {
>    return (obj->size() + java_lang_String::value(obj)->size()) * HeapWordSize;
> }
> 
> 
> #6 Can we rename "StringtableDCmd" to "StringtableDumpCmd”?

Note that "D" here stands for "Diagnostic" and not "Dump", and by 
convention all these types of classes ends with "DCmd".

cheers,
Per

> 
> 
> #7 Isn't "#define PREF_AVG_LIST_LEN   2" a bit too aggressive? Where did the value come from?
> 
> 
> #8 Should we consider adding runtime flag options to control when resizing/cleanup triggers? (i.e. PREF_AVG_LIST_LEN, CLEAN_DEAD_HIGH_WATER_MARK)
> 
> 
> #9 Why do we only resize up, if we can also resize down?
> 
> 
> #10 Do we know the impact of the new table on memory usage (at the default initial size)?
> 
> 
> #11 You mention various benchmarks were ran with no issues, which I take to mean as no regressions, but are there any statistically significant improvements shown that you can report?
> 
> 
> #12 I mentioned this to you off the list, but in a case anyone else tries to run the code - the changes don’t build (which you pointed out to me is due to 8191798). Once things build again, I’d like an opportunity to be able to run the code for myself to check it out, and then report back with final review.
> 
> 
> Cheers
> 
>> On May 28, 2018, at 8:19 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
>>
>> Hi all, please review.
>>
>> This implements the StringTable with the ConcurrentHashtable for managing the
>> strings using oopStorage for backing the actual oops via WeakHandles.
>>
>> The unlinking and freeing of hashtable nodes is moved outside the safepoint,
>> which means GC only needs to walk the oopStorage, either concurrently or in a
>> safepoint. Walking oopStorage is also faster so there is a good effect on all
>> safepoints visiting the oops.
>>
>> The unlinking and freeing happens during inserts when dead weak oops are
>> encountered in that bucket. In any normal workload the stringtable self-cleans
>> without needing any additional cleaning. Cleaning/unlinking can also be done
>> concurrently via the ServiceThread, it is started when we have a high ‘dead
>> factor’. E.g. application have a lot of interned string removes the references
>> and never interns again. The ServiceThread also concurrently grows the table if
>> ‘load factor’ is high. Both the cleaning and growing take care to not prolonging
>> time to safepoint, at the cost of some speed.
>>
>> Kitchensink24h, multiple tier1-5 with no issue that I can relate to this
>> changeset, various benchmark such as JMH, specJBB2015.
>>
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8195097
>> Webrev: http://cr.openjdk.java.net/~rehn/8195097/v0/webrev/
>>
>> Thanks, Robbin
>