Code and Design Feedback request: 7114376: tune system dictionary size

[David Holmes]

Hi Karen,

Just a couple of comments building on Dan's comments.

On 26/01/2012 10:58 AM, Daniel D. Daugherty wrote:
> On 1/25/12 8:55 AM, Karen Kinnear wrote:
>> Folks,
>>
>> Based on your helpful feedback, and help with benchmarking, I have
>> redone the flags and resizing heuristics.
>> Webrev updated at:
>> http://cr.openjdk.java.net/~acorn/7114376.01/webrev/
> src/share/vm/classfile/dictionary.cpp
>
> lines 448-451: if find_unlocked() is called with search criteria
> that doesn't match anything and the SystemDictionary is resized,
> won't we get stuck in this loop since we never refetch an
> updated sdgeneration value?

I agree with Dan, in principle we'll get stuck. But I'm a little puzzled 
as to how, given:

  444   int sdgeneration = SystemDictionary::generation();
  445   int index = hash_to_index(hash);
  446   DictionaryEntry* entry = get_entry(index, hash, name, loader);
  447
  448   while (entry == NULL && SystemDictionary::generation() > 
sdgeneration) {

we can have reached a safepoint between reading the current generation 
at 444 and re-checking it at 448? Is this just being conservative incase 
there is something in hash_to_index or get_entry that might do a 
safepoint check? I presume/assume that get_entry can not be executing 
concurrently with a resize operation.

> lines 444-451:
> I know that HotSpot style does not like do ... while loops, but
> this one seems to be begging for one. Please consider:
>
> 444 DictionaryEntry* entry = NULL;
> 445 do {
> 446   int sdgeneration = SystemDictionary::generation();
> 447   int index = hash_to_index(hash);
> 448   entry = get_entry(index, hash, name, loader);
> 449   // if NULL was returned and generation changed, then a resize
> 450   // happened so we need to try again
> 451 } while (entry == NULL && SystemDictionary::generation() >
> sdgeneration);

+1

>
> nit: line 453 - extra blank after '='
>
> lines 468-474:
> I'm finding the new comment difficult to understand. Please consider:
>
> //
> // With resizable dictionary support, the locking requirements on
> // callers to this routine are a bit trickier:
> // - uses of this routine on shared_system_dictionary and placeholders
> // do not need a lock (these are not resizable)
> // - uses of this routine on resizable dictionaries have to have a lock
> // unless find_unlocked() is used to call this routine
> // - the find_unlocked() routine handles the case where an entry could
> // not be found because the dictionary was resized and does a retry
> // automatically

+1

Cheers,
David