RFR: 8184765: Dynamically resize SystemDictionary

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8184765: Dynamically resize SystemDictionary

Gerard Ziemski
hi all,

Please review this change that adds dynamic resizing of system dictionaries.

The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock

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


cheers

Reply | Threaded
Open this post in threaded view
|

Re: 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
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

coleen.phillimore
In reply to this post by Gerard Ziemski

Gerard, some preliminary comments.

http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html

*+{*
*!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
*!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
*if (probe != NULL) return probe;*
**
*+ }*

I don't think you need this because dictionary->find() should have the
NoSafepointVerifier, so the index will not change.

http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html

I think we want a global _some_dictionary_needs_resizing to avoid
walking through the CLDG.

And have Dictionary have a field _resizing_needed to avoid the
calculation during the safepoint, which is set when adding an entry.

_resizing_needed = *(number_of_entries() >
(_resize_load_trigger*table_size());
*CLDG::_any_resizing_needed |= _resizing_needed;   // or something like
that.

I can write more about the rationale of this change in the bug report,
if needed.

Thank you for doing this change.
Coleen


On 10/10/17 4:40 PM, Gerard Ziemski wrote:

> hi all,
>
> Please review this change that adds dynamic resizing of system dictionaries.
>
> The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock
>
> 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)
> - 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
>
>
> cheers
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Karen Kinnear
A quick note - we are resizing the hash table, i.e. number of linked lists, so as to speed up lookup if it is too small, or waste less space if it is
too large. Each dictionary may have multiple entries off of it - one per loaded class - so there is no such thing as running out
of space based on the sizing (just based on total OOM). I have not yet looked at the change - but just wanted to clarify the
goal and concept for folks first.

thanks
Karen

> On Oct 11, 2017, at 4:10 PM, [hidden email] wrote:
>
>
> Gerard, some preliminary comments.
>
> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
>
> *+{*
> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
> *if (probe != NULL) return probe;*
> **
> *+ }*
>
> I don't think you need this because dictionary->find() should have the NoSafepointVerifier, so the index will not change.
>
> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
>
> I think we want a global _some_dictionary_needs_resizing to avoid walking through the CLDG.
>
> And have Dictionary have a field _resizing_needed to avoid the calculation during the safepoint, which is set when adding an entry.
>
> _resizing_needed = *(number_of_entries() > (_resize_load_trigger*table_size());
> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something like that.
>
> I can write more about the rationale of this change in the bug report, if needed.
>
> Thank you for doing this change.
> Coleen
>
>
> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>> hi all,
>>
>> Please review this change that adds dynamic resizing of system dictionaries.
>>
>> The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock
>>
>> 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)
>> - 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
>>
>>
>> cheers
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

David Holmes
Hi Karen,

On 13/10/2017 8:42 PM, Karen Kinnear wrote:
> A quick note - we are resizing the hash table, i.e. number of linked lists, so as to speed up lookup if it is too small, or waste less space if it is
> too large. Each dictionary may have multiple entries off of it - one per loaded class - so there is no such thing as running out
> of space based on the sizing (just based on total OOM). I have not yet looked at the change - but just wanted to clarify the
> goal and concept for folks first.

Thanks for clarifying - though AFAICS this proposal only grows the
dictionary. Is the "waste less space" to be achieved by starting with a
smaller dictionary initially? Do we have a way to control the initial size?

Thanks,
David

> thanks
> Karen
>
>> On Oct 11, 2017, at 4:10 PM, [hidden email] wrote:
>>
>>
>> Gerard, some preliminary comments.
>>
>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
>>
>> *+{*
>> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
>> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
>> *if (probe != NULL) return probe;*
>> **
>> *+ }*
>>
>> I don't think you need this because dictionary->find() should have the NoSafepointVerifier, so the index will not change.
>>
>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
>>
>> I think we want a global _some_dictionary_needs_resizing to avoid walking through the CLDG.
>>
>> And have Dictionary have a field _resizing_needed to avoid the calculation during the safepoint, which is set when adding an entry.
>>
>> _resizing_needed = *(number_of_entries() > (_resize_load_trigger*table_size());
>> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something like that.
>>
>> I can write more about the rationale of this change in the bug report, if needed.
>>
>> Thank you for doing this change.
>> Coleen
>>
>>
>> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>>> hi all,
>>>
>>> Please review this change that adds dynamic resizing of system dictionaries.
>>>
>>> The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock
>>>
>>> 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)
>>> - 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
>>>
>>>
>>> cheers
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

coleen.phillimore


On 10/13/17 9:11 AM, David Holmes wrote:

> Hi Karen,
>
> On 13/10/2017 8:42 PM, Karen Kinnear wrote:
>> A quick note - we are resizing the hash table, i.e. number of linked
>> lists, so as to speed up lookup if it is too small, or waste less
>> space if it is
>> too large. Each dictionary may have multiple entries off of it - one
>> per loaded class - so there is no such thing as running out
>> of space based on the sizing (just based on total OOM). I have not
>> yet looked at the change - but just wanted to clarify the
>> goal and concept for folks first.
>
> Thanks for clarifying - though AFAICS this proposal only grows the
> dictionary. Is the "waste less space" to be achieved by starting with
> a smaller dictionary initially? Do we have a way to control the
> initial size?

I made the change so that there is one dictionary per class loader
data.  The <bootloader> dictionary starts at 1009 and from what I've
seen in testing has about 900 entries in it on average. So seems
appropriately sized.  The application class loader dictionary (as
default) or dictionary defined with -Djava.system.class.loader and the
other dictionaries start at 107.   The latter can be started with a
larger size with -XX:+PredictedLoadedClassCount.   We had trouble
finding any users who set this option.

The application class loader dictionary is hard to find an initial size
for, and rather than have the user guess which size they want, we will
do automatic resizing to make it larger.  I think the initial size of
application class loader dictionary should be more like 1009.  It was a
bug that I initialized it with 107 actually.

We do not have a way to specify initial size for class loader
dictionaries.   And we don't resize down because that doesn't make any
sense.   Entries are almost never deleted(*), rather the whole
dictionary goes away when the class is unloaded.

So this change is mostly for a class loader that loads a huge number of
classes, that might experience slower than average lookups.  We do have
a user who does this internally.

Coleen

> Thanks,
> David
>
>> thanks
>> Karen
>>
>>> On Oct 11, 2017, at 4:10 PM, [hidden email] wrote:
>>>
>>>
>>> Gerard, some preliminary comments.
>>>
>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html 
>>>
>>>
>>> *+{*
>>> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
>>> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
>>> *if (probe != NULL) return probe;*
>>> **
>>> *+ }*
>>>
>>> I don't think you need this because dictionary->find() should have
>>> the NoSafepointVerifier, so the index will not change.
>>>
>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html 
>>>
>>>
>>> I think we want a global _some_dictionary_needs_resizing to avoid
>>> walking through the CLDG.
>>>
>>> And have Dictionary have a field _resizing_needed to avoid the
>>> calculation during the safepoint, which is set when adding an entry.
>>>
>>> _resizing_needed = *(number_of_entries() >
>>> (_resize_load_trigger*table_size());
>>> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something
>>> like that.
>>>
>>> I can write more about the rationale of this change in the bug
>>> report, if needed.
>>>
>>> Thank you for doing this change.
>>> Coleen
>>>
>>>
>>> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>>>> hi all,
>>>>
>>>> Please review this change that adds dynamic resizing of system
>>>> dictionaries.
>>>>
>>>> The biggest change is refactoring of the code that protects
>>>> calculating of the table entry’s index using SystemDictionary_lock
>>>>
>>>> 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)
>>>> - 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
>>>>
>>>>
>>>> cheers
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

David Holmes
Thanks for the additional background Coleen!

David

On 14/10/2017 12:21 AM, [hidden email] wrote:

>
>
> On 10/13/17 9:11 AM, David Holmes wrote:
>> Hi Karen,
>>
>> On 13/10/2017 8:42 PM, Karen Kinnear wrote:
>>> A quick note - we are resizing the hash table, i.e. number of linked
>>> lists, so as to speed up lookup if it is too small, or waste less
>>> space if it is
>>> too large. Each dictionary may have multiple entries off of it - one
>>> per loaded class - so there is no such thing as running out
>>> of space based on the sizing (just based on total OOM). I have not
>>> yet looked at the change - but just wanted to clarify the
>>> goal and concept for folks first.
>>
>> Thanks for clarifying - though AFAICS this proposal only grows the
>> dictionary. Is the "waste less space" to be achieved by starting with
>> a smaller dictionary initially? Do we have a way to control the
>> initial size?
>
> I made the change so that there is one dictionary per class loader
> data.  The <bootloader> dictionary starts at 1009 and from what I've
> seen in testing has about 900 entries in it on average. So seems
> appropriately sized.  The application class loader dictionary (as
> default) or dictionary defined with -Djava.system.class.loader and the
> other dictionaries start at 107.   The latter can be started with a
> larger size with -XX:+PredictedLoadedClassCount.   We had trouble
> finding any users who set this option.
>
> The application class loader dictionary is hard to find an initial size
> for, and rather than have the user guess which size they want, we will
> do automatic resizing to make it larger.  I think the initial size of
> application class loader dictionary should be more like 1009.  It was a
> bug that I initialized it with 107 actually.
>
> We do not have a way to specify initial size for class loader
> dictionaries.   And we don't resize down because that doesn't make any
> sense.   Entries are almost never deleted(*), rather the whole
> dictionary goes away when the class is unloaded.
>
> So this change is mostly for a class loader that loads a huge number of
> classes, that might experience slower than average lookups.  We do have
> a user who does this internally.
>
> Coleen
>
>> Thanks,
>> David
>>
>>> thanks
>>> Karen
>>>
>>>> On Oct 11, 2017, at 4:10 PM, [hidden email] wrote:
>>>>
>>>>
>>>> Gerard, some preliminary comments.
>>>>
>>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html 
>>>>
>>>>
>>>> *+{*
>>>> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
>>>> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
>>>> *if (probe != NULL) return probe;*
>>>> **
>>>> *+ }*
>>>>
>>>> I don't think you need this because dictionary->find() should have
>>>> the NoSafepointVerifier, so the index will not change.
>>>>
>>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html 
>>>>
>>>>
>>>> I think we want a global _some_dictionary_needs_resizing to avoid
>>>> walking through the CLDG.
>>>>
>>>> And have Dictionary have a field _resizing_needed to avoid the
>>>> calculation during the safepoint, which is set when adding an entry.
>>>>
>>>> _resizing_needed = *(number_of_entries() >
>>>> (_resize_load_trigger*table_size());
>>>> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something
>>>> like that.
>>>>
>>>> I can write more about the rationale of this change in the bug
>>>> report, if needed.
>>>>
>>>> Thank you for doing this change.
>>>> Coleen
>>>>
>>>>
>>>> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>>>>> hi all,
>>>>>
>>>>> Please review this change that adds dynamic resizing of system
>>>>> dictionaries.
>>>>>
>>>>> The biggest change is refactoring of the code that protects
>>>>> calculating of the table entry’s index using SystemDictionary_lock
>>>>>
>>>>> 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)
>>>>> - 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
>>>>>
>>>>>
>>>>> cheers
>>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Gerard Ziemski
In reply to this post by David Holmes
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 <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Gerard Ziemski
In reply to this post by coleen.phillimore
hi Coleen,

Thank you for the review.

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

> On Oct 11, 2017, at 9:10 AM, [hidden email] wrote:
>
>
> Gerard, some preliminary comments.
>
> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
>
> *+{*
> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
> *if (probe != NULL) return probe;*
> **
> *+ }*
>
> I don't think you need this because dictionary->find() should have the NoSafepointVerifier, so the index will not change.

Done, good catch.


> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
>
> I think we want a global _some_dictionary_needs_resizing to avoid walking through the CLDG.
>
> And have Dictionary have a field _resizing_needed to avoid the calculation during the safepoint, which is set when adding an entry.
>
> _resizing_needed = *(number_of_entries() > (_resize_load_trigger*table_size());
> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something like that.
>
> I can write more about the rationale of this change in the bug report, if needed.

Done.


>
> Thank you for doing this change.
> Coleen
>
>
> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>> hi all,
>>
>> Please review this change that adds dynamic resizing of system dictionaries.
>>
>> The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock
>>
>> 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)
>> - 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
>>
>>
>> cheers
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

coleen.phillimore

Hi Gerard,

This looks great.  One small question:

*+ // straight forward brute force*
*+ inline static int _next_prime(int n) {*
*+ int p = n;*
*+ for (int i = n; i < (n * n); i++) {*
*+ if ((i % 2) != 0) {*
*+ p = i;*
*+ break;*
*+ }*
*+ }*
*+ return p;*
*+ }*


Is this how you calculate next prime?  Wouldn't you check if it can mod
by 3 and 5 as well?

Thanks,
Coleen


On 10/27/17 2:25 PM, Gerard Ziemski wrote:

> hi Coleen,
>
> Thank you for the review.
>
> Updated webrev: http://cr.openjdk.java.net/~gziemski/8184765_rev2
>
>> On Oct 11, 2017, at 9:10 AM, [hidden email] wrote:
>>
>>
>> Gerard, some preliminary comments.
>>
>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
>>
>> *+{*
>> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
>> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
>> *if (probe != NULL) return probe;*
>> **
>> *+ }*
>>
>> I don't think you need this because dictionary->find() should have the NoSafepointVerifier, so the index will not change.
> Done, good catch.
>
>
>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
>>
>> I think we want a global _some_dictionary_needs_resizing to avoid walking through the CLDG.
>>
>> And have Dictionary have a field _resizing_needed to avoid the calculation during the safepoint, which is set when adding an entry.
>>
>> _resizing_needed = *(number_of_entries() > (_resize_load_trigger*table_size());
>> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something like that.
>>
>> I can write more about the rationale of this change in the bug report, if needed.
> Done.
>
>
>> Thank you for doing this change.
>> Coleen
>>
>>
>> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>>> hi all,
>>>
>>> Please review this change that adds dynamic resizing of system dictionaries.
>>>
>>> The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock
>>>
>>> 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)
>>> - 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
>>>
>>>
>>> cheers
>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Gerard Ziemski
hi Coleen,


> On Oct 30, 2017, at 10:12 AM, [hidden email] wrote:
>
>
> Hi Gerard,
>
> This looks great.  One small question:
> + // straight forward brute force
> + inline static int _next_prime(int n) {
> +   int p = n;
> +   for (int i = n; i < (n * n); i++) {
> +     if ((i % 2) != 0) {
> +       p = i;
> +       break;
> +     }
> +   }
> +   return p;
> + }
>
> Is this how you calculate next prime?  Wouldn't you check if it can mod by 3 and 5 as well?

As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.

Thank you for the review!


cheers


> Thanks,
> Coleen
>
>
> On 10/27/17 2:25 PM, Gerard Ziemski wrote:
>> hi Coleen,
>>
>> Thank you for the review.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~gziemski/8184765_rev2
>>
>>
>>
>>> On Oct 11, 2017, at 9:10 AM, [hidden email]
>>>  wrote:
>>>
>>>
>>> Gerard, some preliminary comments.
>>>
>>>
>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
>>>
>>>
>>> *+{*
>>> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
>>> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
>>> *if (probe != NULL) return probe;*
>>> **
>>> *+ }*
>>>
>>> I don't think you need this because dictionary->find() should have the NoSafepointVerifier, so the index will not change.
>>>
>> Done, good catch.
>>
>>
>>
>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
>>>
>>>
>>> I think we want a global _some_dictionary_needs_resizing to avoid walking through the CLDG.
>>>
>>> And have Dictionary have a field _resizing_needed to avoid the calculation during the safepoint, which is set when adding an entry.
>>>
>>> _resizing_needed = *(number_of_entries() > (_resize_load_trigger*table_size());
>>> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something like that.
>>>
>>> I can write more about the rationale of this change in the bug report, if needed.
>>>
>> Done.
>>
>>
>>
>>> Thank you for doing this change.
>>> Coleen
>>>
>>>
>>> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>>>
>>>> hi all,
>>>>
>>>> Please review this change that adds dynamic resizing of system dictionaries.
>>>>
>>>> The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock
>>>>
>>>> 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)
>>>> - 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
>>>>
>>>>
>>>>
>>>> cheers
>>>>
>>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

coleen.phillimore


On 10/30/17 11:24 AM, Gerard Ziemski wrote:

> hi Coleen,
>
>
>> On Oct 30, 2017, at 10:12 AM, [hidden email] wrote:
>>
>>
>> Hi Gerard,
>>
>> This looks great.  One small question:
>> + // straight forward brute force
>> + inline static int _next_prime(int n) {
>> +   int p = n;
>> +   for (int i = n; i < (n * n); i++) {
>> +     if ((i % 2) != 0) {
>> +       p = i;
>> +       break;
>> +     }
>> +   }
>> +   return p;
>> + }
>>
>> Is this how you calculate next prime?  Wouldn't you check if it can mod by 3 and 5 as well?
> As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.

If you passed in 214 (2*107), 215 would look prime but it's not.   I
think the prime table would be better and limit resizing occurrences.

Coleen

>
> Thank you for the review!
>
>
> cheers
>
>
>> Thanks,
>> Coleen
>>
>>
>> On 10/27/17 2:25 PM, Gerard Ziemski wrote:
>>> hi Coleen,
>>>
>>> Thank you for the review.
>>>
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~gziemski/8184765_rev2
>>>
>>>
>>>
>>>> On Oct 11, 2017, at 9:10 AM, [hidden email]
>>>>   wrote:
>>>>
>>>>
>>>> Gerard, some preliminary comments.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
>>>>
>>>>
>>>> *+{*
>>>> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
>>>> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
>>>> *if (probe != NULL) return probe;*
>>>> **
>>>> *+ }*
>>>>
>>>> I don't think you need this because dictionary->find() should have the NoSafepointVerifier, so the index will not change.
>>>>
>>> Done, good catch.
>>>
>>>
>>>
>>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
>>>>
>>>>
>>>> I think we want a global _some_dictionary_needs_resizing to avoid walking through the CLDG.
>>>>
>>>> And have Dictionary have a field _resizing_needed to avoid the calculation during the safepoint, which is set when adding an entry.
>>>>
>>>> _resizing_needed = *(number_of_entries() > (_resize_load_trigger*table_size());
>>>> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something like that.
>>>>
>>>> I can write more about the rationale of this change in the bug report, if needed.
>>>>
>>> Done.
>>>
>>>
>>>
>>>> Thank you for doing this change.
>>>> Coleen
>>>>
>>>>
>>>> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>>>>
>>>>> hi all,
>>>>>
>>>>> Please review this change that adds dynamic resizing of system dictionaries.
>>>>>
>>>>> The biggest change is refactoring of the code that protects calculating of the table entry’s index using SystemDictionary_lock
>>>>>
>>>>> 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)
>>>>> - 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
>>>>>
>>>>>
>>>>>
>>>>> cheers
>>>>>
>>>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Andrew Haley
In reply to this post by Gerard Ziemski
On 30/10/17 15:24, Gerard Ziemski wrote:
> As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.
>
> Thank you for the review!

This function is not correctly named.  For example, next_prime(8)
returns 9.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Gerard Ziemski
In reply to this post by coleen.phillimore

> On Oct 30, 2017, at 10:36 AM, [hidden email] wrote:
>
>
>
> On 10/30/17 11:24 AM, Gerard Ziemski wrote:
>> hi Coleen,
>>
>>
>>> On Oct 30, 2017, at 10:12 AM, [hidden email] wrote:
>>>
>>>
>>> Hi Gerard,
>>>
>>> This looks great.  One small question:
>>> + // straight forward brute force
>>> + inline static int _next_prime(int n) {
>>> +   int p = n;
>>> +   for (int i = n; i < (n * n); i++) {
>>> +     if ((i % 2) != 0) {
>>> +       p = i;
>>> +       break;
>>> +     }
>>> +   }
>>> +   return p;
>>> + }
>>>
>>> Is this how you calculate next prime?  Wouldn't you check if it can mod by 3 and 5 as well?
>> As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.
>
> If you passed in 214 (2*107), 215 would look prime but it's not.   I think the prime table would be better and limit resizing occurrences.

Right, need something a bit more clever here. A precomputed prime table might be good enough, but let me see how complex (a correct) function has to be.


cheers
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Gerard Ziemski
In reply to this post by Andrew Haley

> On Oct 30, 2017, at 10:37 AM, Andrew Haley <[hidden email]> wrote:
>
> On 30/10/17 15:24, Gerard Ziemski wrote:
>> As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.
>>
>> Thank you for the review!
>
> This function is not correctly named.  For example, next_prime(8)
> returns 9.

Thanks Andrew, you’re right, the function is not correct - I’m on it.


cheers

Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8184765: Dynamically resize SystemDictionary

Doerr, Martin
In reply to this post by Gerard Ziemski
Hi Gerard,

I guess something like the following was meant.
(I assume overflow will not happen.)

Best regards,
Martin

int next_prime(int n) {
    if (n < 2) return 2;
    int p = (n+1)|1, d=3;
    while (d <= p/d) {
        if (p%d == 0) {
            p+=2; d=3;
        } else {
            d+=2;
        }
     }
    return p;
}

-----Original Message-----
From: hotspot-runtime-dev [mailto:[hidden email]] On Behalf Of Gerard Ziemski
Sent: Monday, October 30, 2017 4:52 PM
To: [hidden email]
Cc: [hidden email]
Subject: Re: RFR: 8184765: Dynamically resize SystemDictionary


> On Oct 30, 2017, at 10:36 AM, [hidden email] wrote:
>
>
>
> On 10/30/17 11:24 AM, Gerard Ziemski wrote:
>> hi Coleen,
>>
>>
>>> On Oct 30, 2017, at 10:12 AM, [hidden email] wrote:
>>>
>>>
>>> Hi Gerard,
>>>
>>> This looks great.  One small question:
>>> + // straight forward brute force
>>> + inline static int _next_prime(int n) {
>>> +   int p = n;
>>> +   for (int i = n; i < (n * n); i++) {
>>> +     if ((i % 2) != 0) {
>>> +       p = i;
>>> +       break;
>>> +     }
>>> +   }
>>> +   return p;
>>> + }
>>>
>>> Is this how you calculate next prime?  Wouldn't you check if it can mod by 3 and 5 as well?
>> As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.
>
> If you passed in 214 (2*107), 215 would look prime but it's not.   I think the prime table would be better and limit resizing occurrences.

Right, need something a bit more clever here. A precomputed prime table might be good enough, but let me see how complex (a correct) function has to be.


cheers
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Gerard Ziemski
In reply to this post by coleen.phillimore
hi Coleen,

I updated the webrev to rev3

bug:
https://bugs.openjdk.java.net/browse/JDK-8184765

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


> On Oct 30, 2017, at 10:36 AM, [hidden email] wrote:
>
>
>
> On 10/30/17 11:24 AM, Gerard Ziemski wrote:
>> hi Coleen,
>>
>>
>>> On Oct 30, 2017, at 10:12 AM, [hidden email] wrote:
>>>
>>>
>>> Hi Gerard,
>>>
>>> This looks great.  One small question:
>>> + // straight forward brute force
>>> + inline static int _next_prime(int n) {
>>> +   int p = n;
>>> +   for (int i = n; i < (n * n); i++) {
>>> +     if ((i % 2) != 0) {
>>> +       p = i;
>>> +       break;
>>> +     }
>>> +   }
>>> +   return p;
>>> + }
>>>
>>> Is this how you calculate next prime?  Wouldn't you check if it can mod by 3 and 5 as well?
>> As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.
>
> If you passed in 214 (2*107), 215 would look prime but it's not.   I think the prime table would be better and limit resizing occurrences.

Thank you for catching the problem.

I went with a simple table of prime numbers (as was used before), since Coleen pointed out to me that the resizing will occur at safe point, so we want to keep the time used to find next prime to a minimum (though resizing is probably so expensive anyways, would it really matter?)


cheers


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

John Rose-3
In reply to this post by Gerard Ziemski
On Oct 30, 2017, at 8:52 AM, Gerard Ziemski <[hidden email]> wrote:
>
> Right, need something a bit more clever here. A precomputed prime table might be good enough, but let me see how complex (a correct) function has to be.

I suggest a precomputed prime table and an inefficient-but-simple
validation that runs only in the debug build, via assert(is_prime(tab[i])).

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

David Holmes
In reply to this post by Gerard Ziemski
Hi Gerard,

The flag should not be experimental - enabling resizable tables is not
an experiment that the user is controlling, it is the default behaviour
that we intend to occur. The flag is there to turn it off if for some
reason this doesn't work - the flag should be a product flag.

The flag also requires a CSR request.

Thanks,
David

On 31/10/2017 9:28 AM, Gerard Ziemski wrote:

> hi Coleen,
>
> I updated the webrev to rev3
>
> bug:
> https://bugs.openjdk.java.net/browse/JDK-8184765
>
> webrev:
> http://cr.openjdk.java.net/~gziemski/8184765_rev3
>
>
>> On Oct 30, 2017, at 10:36 AM, [hidden email] wrote:
>>
>>
>>
>> On 10/30/17 11:24 AM, Gerard Ziemski wrote:
>>> hi Coleen,
>>>
>>>
>>>> On Oct 30, 2017, at 10:12 AM, [hidden email] wrote:
>>>>
>>>>
>>>> Hi Gerard,
>>>>
>>>> This looks great.  One small question:
>>>> + // straight forward brute force
>>>> + inline static int _next_prime(int n) {
>>>> +   int p = n;
>>>> +   for (int i = n; i < (n * n); i++) {
>>>> +     if ((i % 2) != 0) {
>>>> +       p = i;
>>>> +       break;
>>>> +     }
>>>> +   }
>>>> +   return p;
>>>> + }
>>>>
>>>> Is this how you calculate next prime?  Wouldn't you check if it can mod by 3 and 5 as well?
>>> As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.
>>
>> If you passed in 214 (2*107), 215 would look prime but it's not.   I think the prime table would be better and limit resizing occurrences.
>
> Thank you for catching the problem.
>
> I went with a simple table of prime numbers (as was used before), since Coleen pointed out to me that the resizing will occur at safe point, so we want to keep the time used to find next prime to a minimum (though resizing is probably so expensive anyways, would it really matter?)
>
>
> cheers
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184765: Dynamically resize SystemDictionary

Gerard Ziemski
hi David,

I humbly disagree.

The flag was only meant to be there temporarily, and the fact that the feature is turned ON by default here means that turning it OFF would be considered “experimental”, since we don’t want such option to be considered a valid product configuration.

Does that argument convince you?

If not, I’d rather remove the flag altogether.


cheers

> On Oct 30, 2017, at 10:50 PM, David Holmes <[hidden email]> wrote:
>
> Hi Gerard,
>
> The flag should not be experimental - enabling resizable tables is not an experiment that the user is controlling, it is the default behaviour that we intend to occur. The flag is there to turn it off if for some reason this doesn't work - the flag should be a product flag.
>
> The flag also requires a CSR request.
>
> Thanks,
> David
>
> On 31/10/2017 9:28 AM, Gerard Ziemski wrote:
>> hi Coleen,
>> I updated the webrev to rev3
>> bug:
>> https://bugs.openjdk.java.net/browse/JDK-8184765
>> webrev:
>> http://cr.openjdk.java.net/~gziemski/8184765_rev3
>>> On Oct 30, 2017, at 10:36 AM, [hidden email] wrote:
>>>
>>>
>>>
>>> On 10/30/17 11:24 AM, Gerard Ziemski wrote:
>>>> hi Coleen,
>>>>
>>>>
>>>>> On Oct 30, 2017, at 10:12 AM, [hidden email] wrote:
>>>>>
>>>>>
>>>>> Hi Gerard,
>>>>>
>>>>> This looks great.  One small question:
>>>>> + // straight forward brute force
>>>>> + inline static int _next_prime(int n) {
>>>>> +   int p = n;
>>>>> +   for (int i = n; i < (n * n); i++) {
>>>>> +     if ((i % 2) != 0) {
>>>>> +       p = i;
>>>>> +       break;
>>>>> +     }
>>>>> +   }
>>>>> +   return p;
>>>>> + }
>>>>>
>>>>> Is this how you calculate next prime?  Wouldn't you check if it can mod by 3 and 5 as well?
>>>> As long as it’s not even i.e. mod 2 (and strictly speaking larger than 1). 3 and 5 are prime, so no need to check them.
>>>
>>> If you passed in 214 (2*107), 215 would look prime but it's not.   I think the prime table would be better and limit resizing occurrences.
>> Thank you for catching the problem.
>> I went with a simple table of prime numbers (as was used before), since Coleen pointed out to me that the resizing will occur at safe point, so we want to keep the time used to find next prime to a minimum (though resizing is probably so expensive anyways, would it really matter?)
>> cheers

12