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

[Aleksey Shipilev]

On 1/11/19 1:22 AM, 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?

Mmm. I *hope* so. But, since we are reading the source_file into local, NULL-checking it, and only
then using it, whatever happens with the class cache should not have immediate effect, and current
(racy) caller would use the non-NULL value even if cache is being concurrently cleared. (There are
silly C/C++ memory model issues that may still expose us to NULL even after NULL-check, e.g. by
re-reading the memory instead of using the local, but that would break lots of other places too, I
think)

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

Yeah, looks like it. Well, if that is so, we need to do that move in a separate bug and backport it.
But I'd like someone more savvy in whole redefinition deal to see what is up. This patch can wait
that fix, and apply the caching on top.

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

I *guess* that was the tradeoff for returning the nulls transiently while class was momentarily
redefined... This patch tried to maintain whatever current behavior there is.

> A couple of comments on comments:

Thanks, these are fixed in-place.

-Aleksey