RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Peter Levart]

On 8/13/19 3:57 PM, Aleksey Shipilev wrote:
> On 8/13/19 3:33 AM, Mandy Chung wrote:
>> On 8/12/19 5:13 PM, Frederic Parain wrote:
>> 96 // VM adds 1 to the code index to StackFrameInfo::bci field such that
>> 97 // zero (and negative values) indicates invalid BCI. So substract 1.
>>
>> http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.04/
> I still do not understand the purpose of @Stable here. It seems to complicate the whole thing, and
> for what, exactly? Why can't we be happy with just "final int bci"?

final IMO is least correct here. Because with final, two lies are told:
- the field is not final, because it is later modified from native code
- the final modifier's JMM effects are not effective as the field is 
modified after constructor is finished (*)

The field *is* written to only once and with non-default value. So it 
qualifies for @Stable. Unfortunately the optimization effects of @Stable 
do not apply here as the references to StackFrameInfo objects are 
usually not stable. I would be surprised to ever see code like the 
following:

static final StackFrameInfo info = ...

or

class SomeClass {
     static @Stable StackFrameInfo[] infos;

     static {
         ...
         infos = ...
     }
...


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.


Regards, Peter

(*) You may argue that it is a standard practice to modify final fields 
after constructor is finished in the de-serialization, for example. It 
is, yes. But, deserialization does not even run the constructor and 
deserialization uses explicit memory fences that simulate those in the 
constructor. There's no such fences in the StackWalker code. They could 
be added though, if it was desirable to make StackFrameInfo objects safe 
to be published via data race. There's an appropriate place in code 
where such fence could be added: in the 
java.lang.StackStreamFactory.AbstractStackWalker#fetchStackFrames(int) 
method, just after the native fetchStackFrames method returns. This 
would be coarse-grained and would not hurt performance much.


>
> -Aleksey
>