RFR: 8176593: Throwable::getStackTrace performance regression

[coleen.phillimore at oracle.com]

On 3/14/17 3:58 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
>> On Mar 14, 2017, at 12:28 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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.
> Claes is probably off line now, so I’m answering this one for him. :-) java_lang_String::hash_code computes without using the alternate hash method. That works for the shared table. If use_alternate_hashcode is enabled and the string cannot be found within the shared table, it needs to lookup the string in the runtime string table using the alternate hashcode. Otherwise, it could use the wrong hash value and fail to find the string.

I see.  Claes is online and tells me that alt_hash_string() can't be an 
internal function because it calls seed().

So I think the change is good as-is.

thanks!
Coleen
>
> Thanks,
> Jiangli
>
>> 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
>>>>>