RFR: 8184765: Dynamically resize SystemDictionary

[David Holmes]

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?

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?

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?

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

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

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

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.

---

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?

---

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)

---

test/runtime/LoadClass/TestResize.java
test/runtime/LoadClass/TriggerResize.java

Shouldn't these be test/hotspot/jtreg/runtime/LoadClass/... ?

---

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 ?

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

---

Thanks,
David
-----

> 
> cheers
>