Code and Design Feedback request: 7114376: tune system dictionary size
Paul Hohensee
paul.hohensee at oracle.com
Mon Jan 23 13:24:19 PST 2012
Per-software-thread counters would be easier to implement. We don't
really know how many active hw threads we have, and the hw thread
count can change unpredictably or be unknowable (e.g., virtualized
environments).
Paul
On 1/23/12 10:26 AM, Vitaly Davidovich wrote:
>
> Hi Karen,
>
> Regarding the counters David mentioned, is there also any concern with
> them overflowing, on a long-lived jvm process? Would the code deal
> with negative values properly? Or do you not expect this to be a
> concern? The lookup_count is incremented on each lookup so that looks
> a bit suspect to my uninformed eyes.
>
> As for atomic or not, I think the other issue here is if every bit of
> performance is important, might it make sense to have per-cpu counters
> (with sufficient cache line padding) and then aggregate them when
> needed? This would reduce coherence traffic on these writes.
>
> Vitaly
>
> Sent from my phone
>
> On Jan 23, 2012 9:48 AM, "Karen Kinnear" <karen.kinnear at oracle.com
> <mailto:karen.kinnear at oracle.com>> wrote:
>
> David,
>
> Thank you for the detailed feedback. More below.
>
> On Jan 22, 2012, at 8:50 PM, David Holmes wrote:
>
> > Hi Karen,
> >
> > On 22/01/2012 5:04 AM, Karen Kinnear wrote:
> >> Thank you for asking, my mistake. Here is an accessible webrev.
> >>
> >> Webrev: http://cr.openjdk.java.net/~acorn/7114376/webrev/
> <http://cr.openjdk.java.net/%7Eacorn/7114376/webrev/>
> >
> > systemDictionary.cpp:
> >
> > I notice that in systemDictionary the calculation of the index
> has been moved inside the locked regions. This impacted some of
> the SD method signatures, depending on whether they grabbed the
> lock internally, or whether the lock was first acquired
> externally. Seems to me we could avoid a lot of duplicate index
> calculating code if we went a step further and changed all the
> methods to only take the hash and do their own index calculation
> if needed - in particular if find_class did the calculation then I
> think most of the other index calculations would disappear.
>
> Great suggestion - I will do that and send for re-review.
> >
> > 1757 // Check if system dictionary should be resized to speed up
> find() calls
> > 1758 // Default is to check on class unloading
> > 1759 // SystemDictionarySizeIndex flag allows starting with a
> larger initial size
> > 1760 // if SystemDictionaryAverageDepth is non-zero, also check
> on safepoint cleanup
> > 1761 // and allow customers to select acceptable average depth
> to pass
> > 1762 // to verify_lookup_length, otherwise use
> number_of_classes/table_size
> > 1763 // verify_lookup_length compares actual average lookup
> depth: lookup_length/lookup_count
> > 1764 // with theoretical or user-specified.
> > 1765 // In future we may need to periodically calculate
> lookup_length to see
> > 1766 // if we need to force a safepoint to resize system dictionary
> > 1767 void SystemDictionary::should_resize_dictionary(int
> unloaded_count) {
> >
> > I found the above comment block difficult to parse - perhaps
> some Capitals missing to indicate new points/sentences?
> I am going to simplify the flags to just -XX:LoadedClasses=# so
> this comment will be rewritten and
> sent out for re-review.
> >
> > Also should this method assert that either the SD lock is held
> or else we're at a safepoint?
> Will add the assertion that we are at a safepoint.
> >
> > ---
> >
> > dictionary.hpp/cpp
> >
> > + // Change size of system dictionary
> > + // Initially for growing when lookup time takes too long
> > + Dictionary* dictionary_resize(int newsize);
> >
> > Is "system dictionary" distinct from SystemDictionary?
> >
> > I don't like having this as an instance method as it doesn't
> resize the current dictionary but returns a new-one, leaving the
> caller to free the original, yet has the side-effect of clearing
> all entries from the current one. Seems to me we should simply
> have the caller do:
> >
> > Dictionary* newdict = new Dictionary(newsize);
> > olddict->copy_all_entries(newdict);
> > delete olddict;
> > olddict = newdict;
> >
> > though perhaps copy_all_entries should really be move_all_entries?
> I will make that change. dictionary_resize used to do a lot more
> work, but the way it turned out
> your suggestion is much better.
> move_all_entries is a better name.
> >
> > 477 // This routine does not lock the system dictionary.
> > ...
> > 487 // Actually, the locking here is a bit trickier:
> > ...
> > 493 DictionaryEntry* Dictionary::get_entry(int index, unsigned
> int hash,
> > 494 Symbol* class_name,
> > 495 Handle class_loader) {
> > 496 oop loader = class_loader();
> > 497 _lookup_count++;
> > 498
> > 499 for (DictionaryEntry* entry = bucket(index);
> > 500 entry != NULL;
> > 501 entry = entry->next()) {
> > 502 if (entry->hash() == hash && entry->equals(class_name,
> loader)) {
> > 503 return entry;
> > 504 }
> > 505 _lookup_length++;
> >
> > Your now non-debug counters seem to be being updated in a
> non-threadsafe manner - or are we assuming the lock has been taken
> in the caller?
> Good catch. This is called without a lock.
> So the question is - should I make this an atomic update, or since
> these are approximations used for heuristics
> only, add a comment that we actually don't need them to be
> completely accurate? I can measure
> cost of the atomic update.
> >
> >> An alternative possibility for supplying a single flag might
> be: -XX:LoadedClasses=#
> >>
> >> This could be an anticipated count of expected total classes,
> which we could use
> >> internally to set the initial SystemDictionary size. This might
> outlast any internal changes
> >> in data structures.
> >
> > I think this might be a simpler starting point for things.
> Dynamically resizing the SD is transiently a bad thing for everyone:
> > - pause while it happens
> > - excessive memory use while it happens
> >
> > better in my opinion to try to get the initial number of buckets
> right and not dynamically resize.
> I believe we need both. It is simpler to just start with a better
> size - for now I will add the -XX:LoadedClasses=#
> and use that as a starting point, and the only new flag, for
> customers that want to add a command-line argument.
>
> Our goal is to have good performance without requiring custom
> tuning, so the dynamic resizing is valuable for
> customers who do not want to figure out their usage and do not
> want to add a command-line argument.
> I will simplify the ergonomic tuning to be based on the
> dynamically calculated LoadedClasses as well, so
> that if we do resize based on slower lookup times, that we resize
> to the current need rather than incrementally.
>
> I expect the resizing to be beneficial both for customers with
> high numbers of loaded classes and I expect
> the embedded customers to take advantage of resizing to
> potentially shrink the system dictionary. And I tried
> to add the logic so that this could be reused for resizing other
> internal metadata structures.
> >
> > Cheers,
> > David
> > -----
>
> thank you for the very helpful feedback,
> Karen
>
> >
> >> Tests run:
> >> vmsqe: nsk.quick.testlist: solaris-sparc, fastdbg - this
> includes sa testing
> >> jck: vm,lang,api: solaris-sparc, fastdbg
> >> JPRT - this includes CDS testing
> >> performance runs in progress, including frequency of checking
> need to resize
> >>
> >> thanks,
> >> Karen
> >>
> >> On Jan 21, 2012, at 12:04 PM, David Schlosnagle wrote:
> >>
> >>> On Fri, Jan 20, 2012 at 6:06 PM, Karen
> Kinnear<karen.kinnear at oracle.com
> <mailto:karen.kinnear at oracle.com>> wrote:
> >>>> Please review initial proposal for: 7114376: tune system
> dictionary size
> >>>>
> >>>>
> http://oklahoma.us.oracle.com/~kmkinnea/webrev/sdresize/webrev/
> <http://oklahoma.us.oracle.com/%7Ekmkinnea/webrev/sdresize/webrev/>
> >>>
> >>> Karen,
> >>>
> >>> Is there a URL where this webrev is accessible outside of Oracle?
> >>>
> >>> Thanks,
> >>> Dave
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20120123/2eb2c242/attachment.html
More information about the hotspot-dev
mailing list