[RFR] Instrumentation for Unsafe volatile put/get

[Jean Christophe Beyler]

Hi Arthur,

Version 01 looks good to me :)
Jc

On Mon, Sep 23, 2019 at 8:29 AM Arthur Eubanks <aeubanks at google.com> wrote:

> On Fri, Sep 20, 2019 at 5:34 PM Man Cao <manc at google.com> wrote:
>
> > Looks good. No need for new webrev for following nits:
> >
> > Is this change necessary?
> >
> > -  // Default constructors will call this-  AbstractLoop() {++  public
> AbstractLoop() {
> >
> > I don't see why we should restrict it to a specific package.
>
> > I'm not a fan of the names "runInTwoThreadsSync" and "syncSetup". There
> is
> > no obvious extra synchronization compared to runInTwoThreads().
> >
> It's equivalent to adding a barrier at the end of each iteration in each
> thread (I think).
>
> > Maybe "runOnceInTwoThreads" and "runOnceSetup" are better?
> >
> I initially sort of liked this, but "once" is clearly not right, the whole
> point of the setup is that it's run multiple times. I think I'll keep the
> name for now. If somebody wants to change the name in the future, go for
> it.
>
> > Another option might be to create a subclass of AbstractLoop, which
> > overrides runInTwoThreads(). But I don't insist on adding the subclass
> and
> > it's up to you.
> >
> That seems very confusing. I think separating it out into two different
> methods is clearer.
>
> > There could be comments for "runInTwoThreads" and "runOnceInTwoThreads"
> to
> > document their differences.
> >
> Done.
>
> >
> > -Man
> >
> >
> > On Fri, Sep 20, 2019 at 1:15 PM Arthur Eubanks <aeubanks at google.com>
> > wrote:
> >
> >> New webrev:
> >>
> http://cr.openjdk.java.net/~aeubanks/tsanvolatileunsafe/webrev.01/index.html
> >>
> >> On Fri, Sep 20, 2019 at 1:12 PM Arthur Eubanks <aeubanks at google.com>
> >> wrote:
> >>
> >>>
> >>>
> >>> On Fri, Sep 20, 2019 at 11:10 AM Man Cao <manc at google.com> wrote:
> >>>
> >>>> In unsafe.cpp:
> >>>> Calls to __tsan_java_acquire and __tsan_java_release should be
> swapped.
> >>>> I.e., Get*Volatile should use __tsan_java_acquire, and Put*Volatile
> should
> >>>> use __tsan_java_release.
> >>>>
> >>> Oof sorry, bad mistake, fixed. Writing the tests below actually helped
> >>> catch that.
> >>>
> >>>>
> >>>> I think the tests should check that proper use of volatile establishes
> >>>> happens-before, instead of "no race on volatile access". E.g.:
> >>>>
> >>>> volatile byte b = 0;
> >>>> int data = 0;
> >>>> T1:
> >>>> data = 42;
> >>>> b = 1;
> >>>>
> >>>> T2:
> >>>> while (b == 0);
> >>>> int t = data;
> >>>>
> >>> Done. Added some functionality to AbstractLoop.java to properly do
> this.
> >>>
> >>>>
> >>>> In NonRacyUnsafeVolatileLoopTest.java:
> >>>>
> >>>> private byte b = 0x42;
> >>>>
> >>>> Should this field be "volatile"?
> >>>>
> >>> I don't believe these need to be volatile, just the access itself needs
> >>> to be volatile. volatile is a property of a specific memory access,
> not a
> >>> field. Martin agrees.
> >>>
> >>>> -Man
> >>>>
> >>>>
> >>>>
> >>>> On Fri, Sep 20, 2019 at 8:40 AM Arthur Eubanks <aeubanks at google.com>
> >>>> wrote:
> >>>>
> >>>>> webrev:
> >>>>>
> >>>>>
> http://cr.openjdk.java.net/~aeubanks/tsanvolatileunsafe/webrev.00/index.html
> >>>>>
> >>>>> I also added a test that normal volatile accesses (not through
> unsafe)
> >>>>> are
> >>>>> not considered racy, since we didn't have a test for that before.
> >>>>>
> >>>>
>


-- 

Thanks,
Jc