RFR (S) 8216308: StackTraceElement::fill_in can use injected Class source-file

[David Holmes]

On 12/01/2019 9:33 am, coleen.phillimore at oracle.com wrote:
> 
> 
> On 1/10/19 7:22 PM, David Holmes wrote:
>> Hi Aleksey,
>>
>> On 11/01/2019 1:21 am, Aleksey Shipilev wrote:
>>> RFE:
>>>    https://bugs.openjdk.java.net/browse/JDK-8216308
>>>
>>> Fix:
>>>    http://cr.openjdk.java.net/~shade/8216308/webrev.01/
>>>
>>> This is another patch that removes the use of SymbolTable on hot path 
>>> in stack trace creation. We
>>> can inject Class.source_file field to cache the source file name. 
>>> Some caution is needed to properly
>>> handle invalidation when redefinition happens.
>>
>> I'm struggling a bit with the redefinition logic. IIRC redefinition 
>> can only happen at a safepoint so if there are concurrent calls to 
>> fillInStackTrace that involve a given class Foo, then they must all 
>> see the same version of Foo, and we can not have the case where one 
>> execution of the code is clearing the stale cache, while another is 
>> setting it to the new value - right?
>>
>> That said, IIRC Coleen stated that intern can lead to a safepoint, 
>> which would then invalidate the existing redefinition logic because we 
>> would get the line number after the intern and it may now be 
>> incorrect. So I think we have to reorder the code so the 
>> get_line_number occurs before the call to intern.
> 
> In either case, if you redefine the method while calling 
> StringTable::intern, you'll get the line number from the old method. 
> Either before or after the call to String.intern.  It won't crash 
> though, and actually the exception likely happened at the old method's 
> line number.  The only reason we need to specially handle 
> source_file_name is that the redefined class replaces it in the 
> InstanceKlass and we don't have the old one available.

But we already have special handling for the line number if redefinition 
of the method occurred. So we should ensure we don't allow redefinition 
to occur before actually capturing the line number.

>>
>> I'm also very unclear about how the redefinition case is currently 
>> handled. It seems that we will normally intern NULL (and presumably 
>> get a NULL or empty-string oop?) unless ShowHiddenFrames is set, in 
>> which case we use the unknown_class_name() - regardless of whether the 
>> frame is actually hidden or not! This seems broken to me. (Separate 
>> bug to fix that is okay if it is indeed broken.)
> 
> This looks like a bug, but I'm not sure what ShowHiddenFrames is 
> supposed to do here, or how it got there.  I think if Aleksey removed 
> that with this patch it would be fine with me.

I think use of ShowHiddenFrames here is completely broken. But a 
seperate bug and some suitable archaeology is needed to fix it the right 
way.

David

> Coleen
>>
>> A couple of comments on comments:
>>
>> 2616     if (source != NULL) {
>> 2617       // Class was not redefined, can trust its cache.
>> 2618       if (source_file == NULL) {
>>
>> Can you expand the comment as follows:
>>
>> // Class was not redefined. We can trust its cache if set,
>> // else we have to initialize it.
>>
>> 2622     } else {
>> 2623       // Dump the cache in case class had it: it must be have 
>> been redefined.
>> 2624       if (source_file != NULL) {
>>
>> Can you change the comment to be more consistent with the previous one:
>>
>> // Class was redefined. Dump the cache if it was set.
>>
>> Thanks,
>> David
>> -----
>>
>>> This makes stack trace generation significantly faster, and finally 
>>> better than it used to even
>>> before StackWalker and StringTable-related regressions in 9 and 11.
>>>
>>> Benchmark            (depth) Mode Cnt    Score   Error Units
>>>
>>> # 8u
>>> StackTraceBench.test       1 avgt  15   10.851 ± 0.075 us/op
>>> StackTraceBench.test      10 avgt  15   15.325 ± 0.089 us/op
>>> StackTraceBench.test     100 avgt  15   59.717 ± 0.449 us/op
>>> StackTraceBench.test    1000 avgt  15  529.020 ± 3.654 us/op
>>>
>>> # jdk/jdk baseline
>>> StackTraceBench.test      1  avgt  15   15.077 ± 0.065 us/op
>>> StackTraceBench.test     10  avgt  15   21.153 ± 0.123 us/op
>>> StackTraceBench.test    100  avgt  15   80.758 ± 0.363 us/op
>>> StackTraceBench.test   1000  avgt  15  674.888 ± 4.985 us/op
>>>
>>> # jdk/jdk patched
>>> StackTraceBench.test      1  avgt  15    8.892 ± 0.064 us/op
>>> StackTraceBench.test     10  avgt  15   12.010 ± 0.079 us/op
>>> StackTraceBench.test    100  avgt  15   43.091 ± 0.254 us/op
>>> StackTraceBench.test   1000  avgt  15  353.194 ± 2.040 us/op
>>>
>>> Testing: hotspot tier1, jdk-submit, ad-hoc benchmarks
>>>
>>> Thanks,
>>> -Aleksey
>>>
>