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

[Volker Simonis]

On Fri, Jan 11, 2019 at 10:11 AM Aleksey Shipilev <shade at redhat.com> wrote:
>
> 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)

But this scenario (i.e. re-reading a field from memory instead of
using the value cached in a locale) is not related to the memory
model. That depends on the compilers sole discretion and is perfectly
legal C/C++. We've faced such issues several time with XLC on AIX. I
think the only thing that helps is to declare the field "volatile".
This tells the compiler that the value of the field can change at any
time so he won't try to re-read it a second time (and instead spill
its value to the stack if he runs out of registers).

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