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

[Robbin Ehn]

Hi Gerard,

On 2018-06-01 23:46, 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).

Great, thanks!

> 
> 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?

Fixed!

> 
> 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?

Yes, we should, but this involves some start-up benchmarking to see if 'normal' 
workloads will get effected.

> 
> 
> #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));
> 

Fixed (I strongly think DEFAULT_CACHE_LINE_SIZE should be 64 and not 128, that's 
where 64 comes from)

> 
> #3 Extraneous space here, i.e. " name":
> 
> return StringTable::the_table()->do_lookup( name, len, hash);
> 

Fixed

> 
> #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();
> 

Fixed

> 
> #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;
> }
> 

This is a copy of the method from the old hashtable. I think you are correct, 
but I'll let it be for now.

> #7 Isn't "#define PREF_AVG_LIST_LEN   2" a bit too aggressive? Where did the value come from?

The value comes from testing, cleaning and resizing benefits from short chains. 
Since resizing concurrent is right now done with one thread and some care about 
safepoints is taken it's not a particular fast operation. Therefore is much 
preferably to start it before we get long chains.

> 
> 
> #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)
> 

I prefer not, we already have to many flags.

> 
> #9 Why do we only resize up, if we can also resize down?

Shrinking is a bit tricky heuristics-wise, but we could do it in a follow-up yes.

> 
> 
> #10 Do we know the impact of the new table on memory usage (at the default initial size)?

It have the same size per bucket/node as the old hashtable. The difference is 
the 4000 more bucket at default size. I think the footprint benchmark showed 
nothing in differences.

> 
> 
> #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?

The most important one ~2% increase in critical jops specJBB2015.
Regarding the interning it self there is to many parameters to give a simple 
answer. The indirect to oopStorage cost, this means for every Node we need to 
call equals on we get a performance penalty vs old, which is also a reason to do 
aggressive resize. A plain lookup can under optimal condition thus be 25% faster 
in the old hashtable. The fence in global-counter read-side and access api also 
cost.

> 
> 
> #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.

This have been resolved now, I'll send new version to rfr mail.

Thanks, Robbin

> 
> 
> 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
>