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

[Daniel D. Daugherty]

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

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?

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 ','

     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?

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

     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

     nit line 642: you have spaces around one '=' in the output but not the
         other. The original code didn't have spaces around '='.

src/share/vm/classfile/systemDictionary.hpp
nit: 282 - why add a blank line?

     line 548 uses "SystemDictionary"; lines 553 and 591 use
         "system dictionary"; is there some semantic difference?

     line 555: does _sdgeneration have to be an "int" or is it bounded
         by PRIMEARRAYSIZE?

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

src/share/vm/classfile/systemDictionary.cpp
     line 67: if _sdgeneration type in .hpp file changes, then this def
         also has to change.

     nit: line 68: indent off by 1

nit: 86 - why add a blank line?

     nit: deleted blank line on 863 - why?

     lines 1772-1773: comment is not indented, but should be

     line 1780: return seems redundant here

     line 1908: indent of '= 0' is off by 1 space

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?

     nit: 184 - why add a blank line?

src/share/vm/utilities/hashtable.inline.hpp
     nit: deleted blank lines on 67 and 92 - why?

     No other comments.

src/share/vm/utilities/hashtable.cpp
     No comments.