Code and Design Feedback request: 7114376: tune system dictionary size
Vitaly Davidovich
vitalyd at gmail.com
Mon Jan 23 07:26:17 PST 2012
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> 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/
> >
> > 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> wrote:
> >>>> Please review initial proposal for: 7114376: tune system dictionary
> size
> >>>>
> >>>> http://oklahoma.us.oracle.com/~kmkinnea/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/65a72b00/attachment-0001.html
More information about the hotspot-dev
mailing list