Much cleaner! Thumbs up!
Fred > On Aug 14, 2019, at 15:42, Mandy Chung <mandy.ch...@oracle.com> wrote: > > I have further discussion with Coleen and walkthrough the vframe > implementation. Here is what we confirm and agree. > > vframeStream::bci might return an invalid bci whereas javaVFrame::bci returns > a valid bci (compiledVFrame::bci adjusts invalid bci to 0). > > An invalid bci could happen when hitting a safepoint on bci #0 on a > synchronized method either before or after the monitor is locked (implicit > synchronization without explicit monitorenter). That might occur when > StackOverflowError is thrown for example. However, that should never happen > when StackWalker is called and the top frame would be in the stack walking > code. > > +1/-1 adjustment intends to address invalid bci case but such case is not > applicable for StackWalker API. With that, I agree with Coleen that VM > should set the bci value to StackFrameInfo::bci field and no adjustment > needed: > http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.05/ > > thanks > Mandy > > On 8/13/19 4:56 PM, coleen.phillim...@oracle.com wrote: >> >> Hi, I saw my name in this thread and had a discussion with Mandy. I don't >> like that the VM and JDK having this special coordinated dance of +1/-1, and >> the reason for this is to differentiate the value of 0 in StackFrame meaning >> either uninitialized or invalid. If through some race, an unitialized value >> is observed, then the MemberName may not be filled in either. In any case >> zero makes sense to return as the bci because it'll point to the line number >> beginning of the method, if used to get the line number. >> >> The stackwalk API doesn't return invalid bci because it adjusts it: >> >> int compiledVFrame::bci() const { >> int raw = raw_bci(); >> return raw == SynchronizationEntryBCI ? 0 : raw; >> } >> >> At any rate, the 04 webrev looks good minus the +1/-1 dance and >> StackWalker.java comment. Coupling the jdk and jvm like this feels like >> it's asking for bugs. I like the simpler implementation that fixes the bug >> that we have. >> >> Thanks, >> Coleen >> >> >> On 8/13/19 1:49 PM, Mandy Chung wrote: >>> >>> >>> On 8/13/19 9:30 AM, Peter Levart wrote: >>>> Usually the StackFrameInfo(s) are consumed as soon as they are returned >>>> from StackWalker API and never assigned to @Stable field. So there's no >>>> purpose of @Stable for bci field use. Except documentation. But >>>> documentation can be specified in the form of comments too. >>>> >>> >>> There are several JDK classes whose fields are filled by VM and comment is >>> the form of documentation. Until new use of StackFrame in the future shows >>> performance benefit, it's okay to stick with @Stable original intent and >>> keep the comment: >>> >>> + private int bci; // zero and negative values >>> represent invalid BCI >>> >>> Please also review CSR: >>> https://bugs.openjdk.java.net/browse/JDK-8229487 >>> >>> Mandy >> >
