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

[Karen Kinnear]

Dan & David - 

Thank you both for the detailed reviews.

I will make the changes suggested - but I will put the code changes on hold. I think
the best approach right now is to just add the command-line flag and then after
we get the permanent generation changes, rethink the loaded class cache storage
options - and do a whole lot more measurements than I am able to do in this timeframe -
even thanks to the help I have had.

Details below,
thanks,
Karen

On Jan 25, 2012, at 7:58 PM, 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/
> 
> - feel free to ignore the nits
> - thanks for updating the copyright lines
Teachable :-)
> 
> agent/src/share/classes/sun/jvm/hotspot/memory/LoaderConstraintTable.java
>    No comments.
> 
> agent/src/share/classes/sun/jvm/hotspot/memory/SystemDictionary.java
>    Should there be support for fetching and storing the new
>    _sdgeneration and _primelist fields here?
good point. I will save the comment for when we revisit this.
> 
> src/share/vm/classfile/dictionary.hpp
>    line 108: my brain is wondering why rename find() to find_unlocked()
> 
> src/share/vm/classfile/dictionary.cpp
>    Now I see why find() was renamed to find_unlocked().
:-)
> 
>    nit: line 439 - extra blank before ','
fixed.
> 
>    line 443 - do you want to assert() that the SD lock is not held?
> 
>    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?
Thank you - I will update the sdgeneration each time through. And
yes, David, this is paranoia in case of a safepoint or a change in when
we do the resizing in future.
> 
>    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);
Much better - thanks!
> 
>    nit: line 453 - extra blank after '='
fixed.
> 
>    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
// They also need to calculate the index while holding the lock, since
// the index might change if the system dictionary is resized.
>    // - the find_unlocked() routine handles the case where an entry could
>    //   not be found because the dictionary was resized and does a retry
>    //   automatically
Updated - and thanks.
> 
>    nit line 642: you have spaces around one '=' in the output but not the
>        other. The original code didn't have spaces around '='.
removed spaces.
> 
> src/share/vm/classfile/systemDictionary.hpp
> nit: 282 - why add a blank line?
fixed
> 
>    line 548 uses "SystemDictionary"; lines 553 and 591 use
>        "system dictionary"; is there some semantic difference?
made all lower case
> 
>    line 555: does _sdgeneration have to be an "int" or is it bounded
>        by PRIMEARRAYSIZE?
I'll leave this for now and save the comment for when we revisit this.
> 
>    line 592: if _sdgeneration type changes, then return type should
>        change also.
> 
>    lines 668, 671: instead of "unsigned int", should the hash parameter
>        be a new typedef'ed value to make future experiments easier?
>        However, changing that would require updates in many places...
Not touching hash parameter. We want to leave that as unsigned int.
> 
> src/share/vm/classfile/systemDictionary.cpp
>    line 67: if _sdgeneration type in .hpp file changes, then this def
>   also has to change.
True. Also true for all values initialized here.
> 
>    nit: line 68: indent off by 1
fixed
> 
> nit: 86 - why add a blank line?
fixed.
> 
>    nit: deleted blank line on 863 - why?
fixed.
> 
>    lines 1772-1773: comment is not indented, but should be
fixed.
> 
>    line 1780: return seems redundant here
Thanks - it used to return a value.
> 
>    line 1908: indent of '= 0' is off by 1 space
fixed.
> 
> src/share/vm/runtime/globals.hpp
>    No comments.
> 
> src/share/vm/runtime/vmStructs.cpp
>    No comments.
> 
> src/share/vm/utilities/hashtable.hpp
>    nit: 150     if (NULL != _buckets) {
>        This: 'if (_buckets != NULL)' seem more traditional.
>        Any particular reason for your order?
I put it to the more traditional. I tend to like to do my NULL comparisons backwards,
particularly for the == case for the times I accidentally type =. But no big deal.
> 
>    nit: 184 - why add a blank line?
Fixed. code moving/removing.
> 
> src/share/vm/utilities/hashtable.inline.hpp
>    nit: deleted blank lines on 67 and 92 - why?
Changes that didn't make the latest version. Undone.
> 
>    No other comments.
> 
> src/share/vm/utilities/hashtable.cpp
>    No comments.
>