RFR: 8176593: Throwable::getStackTrace performance regression

[coleen.phillimore at oracle.com]

Hi Claes,
This is a good find and very nice that it improves performance with just 
refactoring.

I think you could remove these (2):

+ if (use_alternate_hashcode()) {
+ hash = alt_hash_string(name, len);
+ }

Because there isn't a safepoint in lookup_shared, is there?  So the 
hashing won't change.  Please correct me if I'm wrong.

When we add the string in basic_add, after taking the StringTable_lock, 
we recheck the hashcode there.

I like your alt_hash_string function though, but I think it and 
"hash_string" should be internal to stringTable.cpp (and not in the .hpp 
file) because they're not public and only called from stringTable.cpp.

Thank you for fixing this!
Coleen

On 3/14/17 2:09 PM, Claes Redestad wrote:
> Hi Jiangli,
>
> On 2017-03-14 18:22, Jiangli Zhou wrote:
>> Hi Claes,
>>
>> Thank you for looking into this! The changes look good to me. I have
>> some small suggestions below.
>>
>> I’d suggest to add a comment explaining the
>> java_lang_String::hash_code() call before lookup_shared(). For example,
>> “shared table requires java_lang_String::hash_code()”.
>
> Sure!
>
>>
>> How about changing the following hash_string() calls to
>> AltHashing::murmur3_32() directly? That would avoid the second
>> use_alternate_hashcode() check in hash_string().
>>
>> 206 if (use_alternate_hashcode()) {
>> 207 hash = hash_string(name, len);
>> 208 }
>>
>> 224 if (use_alternate_hashcode()) {
>> 225 hashValue = hash_string(name, len);
>> 226 }
>
> I'm not so sure manually inlining matters here, but I haven't inspected
> the disassembly, but with a little refactoring and it arguably makes the
> intent clearer:
>
> http://cr.openjdk.java.net/~redestad/8176593/hotspot.02/
> http://cr.openjdk.java.net/~redestad/8176593/hotspot.01.02.diff/
>
> Thanks!
>
> /Claes
>
>>
>> Thanks,
>> Jiangli
>>
>>> On Mar 14, 2017, at 9:57 AM, Claes Redestad <claes.redestad at oracle.com
>>> <mailto:claes.redestad at oracle.com>> wrote:
>>>
>>> Hi,
>>>
>>> current implementation of StringTable::lookup_shared computes hash
>>> values twice even when the same hash algorithm is used, which stand out
>>> in profiles as the primary cause for a performance regression on things
>>> like Throwable::getStackTrace.
>>>
>>> By refactoring the logic to not compute hashes using a specific
>>> algorithm more than once we recuperate the regression and actually beat
>>> JDK 8 on reported microbenchmarks.
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8176593/hotspot.01/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176593
>>>
>>> Testing: RBT jdk-tier1,hs-runtime-pre-checkin,hs-tier2
>>>
>>> Thanks!
>>>
>>> /Claes
>>>
>>