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

[David Holmes]

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.

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.)

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
>