RFR: 8191786: Thread-SMR hash table size should be dynamic [v2]

[Coleen Phillimore]

On Tue, 25 May 2021 19:25:21 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> src/hotspot/share/runtime/threadSMR.cpp line 204:
>> 
>>> 202:   //   "BasicHashtable<(MEMFLAGS)2>::new_entry(unsigned int)", referenced from:
>>> 203:   //   KVHashtable<void*, bool, (MEMFLAGS)2, &(unsigned int primitive_hash<void*>(void* const&)), &(bool primitive_equals<void*>(void* const&, void* const&))>::add_if_absent(void*, bool, bool*) in threadSMR.o
>>> 204:   //
>> 
>> We don't need these details in the comment.
>
> True, but they served their purpose! I'm going to check out the fix
> that @robehn posted about how to get past this compilation issue.

I don't think you should have these comments either.  They don't really help me understand why you get this compilation error.  It seems like a TODO.  Maybe the mtThread instantiation is missing from hashtable.cpp?

>> src/hotspot/share/runtime/threadSMR.cpp line 854:
>> 
>>> 852: 
>>> 853: // Hash table size should be first power of two higher than twice the
>>> 854: // length of the ThreadsList
>> 
>> What is the basis for this? Seems reasonable for small numbers of threads but excessive for large numbers.
>
> This is where we need @fisk. This comment and the associated code was
> written by @fisk in the early days of the Thread-SMR project. So we need
> Erik to go spelunking in the recesses of his memory for why he sized things
> this way...
> 
> Also, I think the comment is a bit wrong and doesn't match the code below.

The theory in the code for KVHashtable or BasicHashtable is that it works better for prime numbers.  It'll resize to other prime numbers if needed.  I assume starting with 1007 should work fine, if the number of entries matches the number of threads.

const int _small_table_sizes[] = { 107, 1009, 2017, 4049, 5051, 10103, 20201, 40423 } ;
These are the resizing sizes in hashtable.cpp.

Edit: You could use this table to choose an initial size based on your current number of threads.

>> src/hotspot/share/runtime/threadSMR.cpp line 857:
>> 
>>> 855: static int hash_table_size() {
>>> 856:   ThreadsList* threads = ThreadsSMRSupport::get_java_thread_list();
>>> 857:   int hash_table_size = MIN2((int)threads->length(), 32) << 1;
>> 
>> Doesn't this start things off a bit small. If the old size was 1031 why not start in that area?
>
> We need @fisk to time in here with his rationale from the beginning of the Thread-SMR project.
> 
> The 1031 came from me and I stole it from some other fixed hash table size in the VM.
> 
> Someone please correct me if I'm wrong, but doesn't this hash_table_size value max out
> at 64? `MIN2((int)threads->length(), 32)` => `32` when there are more than 32 threads
> and `32 << 1` => `64`.
> 
> Yeah, we really need @fisk to chime in here.
> 
> Also, don't forget that these hash_tables are very, very short lived
> since they only exist while we are gathering either hazard ptrs or
> JavaThread* protected by hazard ptrs.

Yes, 32 doesn't seem right.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4168