[9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test (original) (raw)

Vitaly Davidovich vitalyd at gmail.com
Tue Feb 25 06:13:15 PST 2014

Hi Albert,

There's currently one instantiation of the hashtable that looks like this:

new GenericHashtable<DependencySignature, ResourceObj>(11027);

With size as template, it would be: new GenericHashtable<DependencySignature, ResourceObj, 11027>();

So the size is specified in the same place (in terms of reading the code). Your call though, this was just a suggestion.

Power of 2 hashtable sizes are an issue with poor quality hash functions (I.e. ones that don't differ in low order bits), but if the hash function is decent it shouldn't be an issue. Prime size is used to mitigate that, but there are other techniques that allow 2^n sizing (e.g. supplementary hash function applied on top of hash code coming from original key object). FWIW, JDK's HashMap is 2^n sized with supplementary hashing.

Again, just a suggestion - it probably won't make a substantial difference here.

Thanks

Sent from my phone On Feb 25, 2014 8:06 AM, "Albert" <albert.noll at oracle.com> wrote:

Hi,

Vladimir, Christian, Vitaly, thanks for looking at this and your feedback. @Vladimir: No, this change is based on the version of 7194669. However, the diff to before 7194669 are mainly code refactorings, which make the code more readable (for me). I have changed the parameter name, (bool Cheap = false), adapted the 'add' function according to your suggestion, and implemented the hashtable destructor as well as the remove function. @Christian: This for noticing this inconsistency. I fixed the parameter names @Vitaly: I would prefer to leave the size parameter as it is now. While we would save some instructions, I think that specifying the size of the hashtable where it is used makes the code more readable. Shouldn't we, in general, try to avoid hash table sizes that are an exact power of 2? Here is the new webrev: http://cr.openjdk.java.net/~anoll/8034939/webrev.01/ Best, Albert

On 02/21/2014 11:54 PM, Christian Thalinger wrote: On Feb 21, 2014, at 11:27 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

Lets discuss it on hotspot-dev.

Note the current hashtable allocates only in cheap. Albert added hashtable which can allocate in thread local resource area for temporary table and cheap for long live table. Albert, So you restored code in dependencies.?pp to one before 7194669 fix. Right? I think you need to follow GrowableArray example to name parameter "bool Cheap = false" instead of "bool resourcemark". It should be saved in a field because you need to free cheap in destructor if C-heap is used: ~GrowableArray() { if (onCheap()) clearanddeallocate(); } Also I think you should avoid call to contains(item) in add() to avoid doing the same thing twice. ...and you should stick to either item or element: + template<class T, class F> bool GenericHashtable<T, F>::add(T* item) { + template<class T, class F> bool GenericHashtable<T, F>::contains(T* element) { You should implement remove(). Thanks, Vladimir On 2/21/14 12:04 AM, Albert wrote:

Hi,

could I get reviews for this small patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8034839 Problem: The problem is that the patch (7194669) - which was supposed to speed-up dependency checking causes a performance regression. The reason for the performance regression is that most dependencies are unique, so we have the overhead of determining if the dependency is already checked plus the overhead of dependency checking. The overhead of searching is significant, since we perform a linear search on 6000+ items each time. Solution: Use a hashtable instead of linear search to lookup already checked dependencies. The new hashtable is very rudimentary. It provides only the required functionality to solve this bug. However, the functionality can be easily extended as needed. Testing: jprt, failing test case, nashorn. The failing test case completes in approx. the same time as before 7194669. For nashorn + Octane, this patch yields the following times spent for dependency checking: with this patch: 844s 7194669: 1080s before 7194669: 5223s webrev: http://cr.openjdk.java.net/~anoll/8034939/webrev.00/ Thanks, Albert

More information about the hotspot-dev mailing list