RFR: 8184765: Dynamically resize SystemDictionary

[Gerard Ziemski]

hi David,

Thank you for your detailed review and questions - my answers below.

Updated webrev: http://cr.openjdk.java.net/~gziemski/8184765_rev2


> On Oct 11, 2017, at 12:07 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Gerard,
> 
> On 11/10/2017 6:40 AM, Gerard Ziemski wrote:
>> hi all,
>> Please review this change that adds dynamic resizing of system dictionaries.
> 
> Sounds good - but there is nothing in the bug report to justify any of the resizing choices - eg resize factor - where are the performance numbers? :)
> 
> In the JDK libraries the policy of always doubling collections when they needed to grow was removed because as the collection gets really big, doubling its size becomes a major problem. What kind of sizes are we likely to be dealing with?

This is not as much about improving performance of general apps, but more about uncommon cases for big apps (i.e. those loading 40,000 classes)


> Given we don't currently resize, when will we now see resizing happening? ie what apps will be affected by this and what performance hit might they take when the resizing occurs?

Resizing will take place at a safepoint when the system dictionary’s underlaying hash table load factor is too high (currently it’s 5, i.e. we will resize when we reach an average of 5 entries per a bucket)


> 
> IIUC resizing is done lazily if we hit the right thresholds when a safepoint happens. I'm unclear what kind of margins we have to ensure we are not likely to run out of space before the next safepoint might occur?

As Karen explained, we can run out of memory, not entries - if that happens, we keep system dictionaries at their old sizes.


> 
>> The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock
> 
> So basically anyone calling hash_to_index must now hold the SD lock when doing that call. In most places it is a simple matter of moving the call to a locked region later in the method; but in SD::resolve_instance_class_or_null you added a new locked region - I am concerned this may introduce a synchronization bottleneck.

I ran various performance benchmarks (including specjvm via Aurora) and there were no regressions.


> 
>> A few notes:
>> - dynamic resizing is off when either dumping or using shared spaces
>> - we remove the experimental runtime option “PredictedLoadedClassCount” and add “DynamicallyResizeSystemDictionaries” to turn the resizing off (it’s on by default)
> 
> I don't think it makes sense to make a flag that restores the old behaviour, experimental. This should be a product flag IMHO. This flag is different to PredictedLoadedClassCount which provided an unsupported (aka experimental) means to influence the initial table size.

I heard a different argument calling for no flag at all. IMHO there should either be no flag - we want resizable system dictionaries after all, and they are ON by default, but since this is a new feature I thought it would be safer to be able to turn it off, if it causes issues for the customers out in the real world.


> 
>> - the jtreg test uses stream of bytes to dynamically load numbered classes from memory instead of disk (thank you Ioi)
>> bug:    https://bugs.openjdk.java.net/browse/JDK-8184765
>> webrev: http://cr.openjdk.java.net/~gziemski/8184765_rev1
> 
> src/hotspot/share/classfile/dictionary.cpp
> 
> 109   for (int i=n; i<n*n; i++) {
> 110     if (i%2 != 0) {

Done.


> 
> Style nits: put spaces around all operators please (check all files)
> 
> 123         desired_size = _next_prime((int)(_resize_factor*(double)number_of_entries()));
> 
> You don't need the cast to double; the fact _resize_factor is a double means the calculation will be done as a double.

Done.


> 
> ---
> 
> src/hotspot/share/utilities/hashtable.cpp
> 
> 271   HashtableBucket<F>* buckets_new = NEW_C_HEAP_ARRAY2(HashtableBucket<F>, new_size, F, CURRENT_PC);
> 272   if (buckets_new == NULL) {
> 
> You're using the NEW_ macro that aborts on OOM not the one that returns NULL.
> 
> BTW what happens when the SD does fill up? Do we abort or just stop loading classes?

Done.


> 
> ---
> 
> src/hotspot/share/utilities/hashtable.hpp
> 
> May be worth adding:
> 
> assert_locked_or_safepoint(SystemDictionary_lock);
> 
> to hash_to_index (or just a lock ownwership test if you don't expect this to be done at a safepoint)

Other hash tables use it too and may not be calling it with a lock or at a safepoint (yet).


> 
> ---
> 
> test/runtime/LoadClass/TestResize.java
> test/runtime/LoadClass/TriggerResize.java
> 
> Shouldn't these be test/hotspot/jtreg/runtime/LoadClass/… ?

Fixed, thank you for catching this.


> 
> ---
> 
> TestResize.java
> 
>  24 /*
>  25  * @test
>  26  * @bug 8184765
>  27  * @summary Test dynamic resizing of SystemDictionary
>  28  * @modules java.base/jdk.internal.misc:open
>  29  * @modules java.base/java.lang:open
>  30  * @library /test/lib
>  31  */
> 
> This isn't actually a test - you never run it directly - so I don't expect to see any jtreg tags here ?? I would expect them all to be on the actual test class. Actually I'm not sure why this needs to be a separate public class in its own file ?

Done. I followed the pattern, but also IMHO it’s cool to be able to run the test case on its own.


> 
> Please add comments in load() explaining what all the byte/index stuff is doing - presumably making the class representation unique?

Done.


cheers