harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiao-Feng Li" <xiaofeng...@gmail.com>
Subject Re: [jira] Updated: (HARMONY-2092) [drlvm][jit] Harmony works with volatile variables incorrectly
Date Wed, 23 May 2007 12:50:59 GMT
On 5/23/07, Mikhail Fursov <mike.fursov@gmail.com> wrote:
> On 5/23/07, Xiao-Feng Li <xiaofeng.li@gmail.com> wrote:
> >
> > I had a question in the JIRA about this issue: why don't we use "lock"
> > prefix for the atomic access?
>
>
> Lock prefix will cause significant performance degradation. For example,
> some time ago, when our monenter implementation always used locks the
> inlining of this helper gave no benefit at all. The removal of lock without
> inlining gave a real benefit but was illegal.

Mikhail, actually I was working on synchronization for a pretty long
time, so the "lock" prefix implication to performance is well
understood. In my understanding, volatile long is quite different from
Java monitor. I personally don't believe that to use "lock" prefix for
volatile long can cause any visible performance issue in real
workloads, if it doesn't bring performance improvement. (Probably JIT
can do a good job here to optimize the excessive "lock" prefixes if
the volatile long variable really shows performance issue, although
it's unlikely to happen in real workloads.)

My concern is the 8-byte object alignment requirement if we use
current solution. It requires to change the class loader and object
arrangement in heap. For example, do we have any performance study on
its implication to SPECs?  Probably the 8-byte alignment would
ultimately be needed for Harmony, but there is a methodology issue
here: Currently the problem with volatile long is its correctness,
instead of its performance. We don't need consider too much about the
performance issue if we can achieve the correctness relatively
trivially and quickly (by simply emitting a "lock" prefix). Then we
can check if the performance is indeed an issue. We still keep the
optimization opportunities, and it's not late to implement current
solution after we have a good understanding in the issue.

These are just my cents, for your reference. :-)

> AFAIK we do not have problems with volatile int32/int16/int8 types today on
> 32bit PC. If this is true we can try to handle int64 values using 64bit
> registers without locks too.

Yes, as I described above, it's good to have the try, but we need
consider more. For example, do we know if it will introduce any
portability issue?

Thanks,
xiaofeng

>
>
> Thanks,
> > xiaofeng
> >
> > On 5/23/07, George Timoshenko (JIRA) < jira@apache.org> wrote:
> > >
> > >      [ https://issues.apache.org/jira/browse/HARMONY-2092?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
> > ]
> > >
> > > George Timoshenko updated HARMONY-2092:
> > > ---------------------------------------
> > >
> > >     Attachment: HARMONY-2092-regression_test.patch
> > >
> > > regression test
> > > based on the testcase by Mikhail Fursov
> > >
> > > > [drlvm][jit] Harmony works with volatile variables incorrectly
> > > > --------------------------------------------------------------
> > > >
> > > >                 Key: HARMONY-2092
> > > >                 URL:
> > https://issues.apache.org/jira/browse/HARMONY-2092
> > > >             Project: Harmony
> > > >          Issue Type: Bug
> > > >          Components: DRLVM
> > > >         Environment: Windows and Linux
> > > >            Reporter: Vera Petrashkova
> > > >         Assigned To: weldon washburn
> > > >         Attachments: classloader_and_gc_quick_fix.diff,
> > HARMONY-2092-regression_test.patch, HARMONY-2092.patch, volatileTest.zip
> > > >
> > > >
> > > > When Long_MAX_VALUE or Long.MIN_VALUE should be assigned to volatile
> > variable then
> > > > VM works incorrectly.
> > > > Code for reproducing:
> > > > ------------------volatileTest.java-----------------
> > > > public class volatileTest  {
> > > >     public static int delay = 5000;
> > > >     private boolean interruptFlag = false;
> > > >     long hi;
> > > >     long lo;
> > > >     volatile long v;
> > > >     public static void main(String [] args ) {
> > > >         int maxI = 20;
> > > >         for (int i = 0; i < maxI; i++) {
> > > >             int t = new volatileTest().test(delay);
> > > >             System.out.println((t == 104 ? "Test passed: ": "Test
> > failed: ")
> > > >                 + t + "  (step: " + i + ")");
> > > >         }
> > > >     }
> > > >     public synchronized void interrupt() {
> > > >         interruptFlag = true;
> > > >     }
> > > >     public synchronized boolean interrupted() {
> > > >         return interruptFlag;
> > > >     }
> > > >     public volatileTest() {
> > > >         super();
> > > >         hi = Long.MAX_VALUE;
> > > >         lo = Long.MIN_VALUE;
> > > >         v = hi;
> > > >     }
> > > >     public int test(int delay)  {
> > > >         boolean passed = true;
> > > >         Thread_1_class  t1 = new Thread_1_class(this);
> > > >         Thread_2_class t2 = new Thread_2_class(this);
> > > >         Interruptor killer = new Interruptor(this, delay);
> > > >         try {
> > > >             t1.start();
> > > >             t2.start();
> > > >             killer.start();
> > > >
> > > >             while (!interrupted()) {
> > > >                 Thread.yield();
> > > >                 long v3 = v;
> > > >                 if (v3 != hi && v3 != lo) {
> > > >                     System.out.println(v3+"  "+hi +"  "+lo);
> > > >                     passed = false;
> > > >                     break;
> > > >                 }
> > > >             }
> > > >             t1.interrupt();
> > > >             t2.interrupt();
> > > >             return passed ? 104 : 105;
> > > >        } catch (Throwable e) {
> > > >            e.printStackTrace();
> > > >            return 106;
> > > >        }
> > > >     }
> > > > }
> > > > class Thread_1_class extends Thread {
> > > >    volatileTest thh;
> > > >    public Thread_1_class (volatileTest t) {
> > > >        super();
> > > >        thh = t;
> > > >    }
> > > >    public void run() {
> > > >        while (!isInterrupted()) {
> > > >            thh.v = thh.lo;
> > > >            thh.v = thh.hi;
> > > >        }
> > > >    }
> > > > }
> > > > class Thread_2_class extends Thread {
> > > >    volatileTest thh;
> > > >    public Thread_2_class (volatileTest t) {
> > > >        super();
> > > >        thh = t;
> > > >    }
> > > >    public void run() {
> > > >        while (!isInterrupted()) {
> > > >                 thh.v = thh.hi;
> > > >                 thh.v = thh.lo;
> > > >             }
> > > >    }
> > > > }
> > > > class Interruptor extends Thread {
> > > >     volatileTest t;
> > > >     int delay;
> > > >     public Interruptor(volatileTest t, int delay) {
> > > >         this.t = t;
> > > >         this.delay = delay;
> > > >     }
> > > >     public void run() {
> > > >         try {
> > > >             Thread.sleep(delay);
> > > >         } catch (InterruptedException e) {
> > > >         }
> > > >         t.interrupt();
> > > >     }
> > > > }
> > > > ----------------------------
> > > > Output on RI:
> > > > ================
> > > > java version " 1.5.0_06"
> > > > Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_06-b05)
> > > > Java HotSpot(TM) Client VM (build 1.5.0_06-b05, mixed mode)
> > > > Test passed: 104  (step: 0)
> > > > Test passed: 104  (step: 1)
> > > > Test passed: 104  (step: 2)
> > > > Test passed: 104  (step: 3)
> > > > Test passed: 104  (step: 4)
> > > > Test passed: 104  (step: 5)
> > > > Test passed: 104  (step: 6)
> > > > Test passed: 104  (step: 7)
> > > > Test passed: 104  (step: 8)
> > > > Test passed: 104  (step: 9)
> > > > Test passed: 104  (step: 10)
> > > > Test passed: 104  (step: 11)
> > > > Test passed: 104  (step: 12)
> > > > Test passed: 104  (step: 13)
> > > > Test passed: 104  (step: 14)
> > > > Test passed: 104  (step: 15)
> > > > Test passed: 104  (step: 16)
> > > > Test passed: 104  (step: 17)
> > > > Test passed: 104  (step: 18)
> > > > Test passed: 104  (step: 19)
> > > > Output on Harmony:
> > > > ====================
> > > > Apache Harmony Launcher : (c) Copyright 1991, 2006 The Apache Software
> > Foundation or its licensors, as applicable.
> > > > java version " 1.5.0"
> > > > pre-alpha : not complete or compatible
> > > > svn = r471468, (Nov  7 2006), Windows/ia32/msvc 1310, release build
> > > > http://incubator.apache.org/harmony
> > > > Test passed: 104  (step: 0)
> > > > Test passed: 104  (step: 1)
> > > > Test passed: 104  (step: 2)
> > > > Test passed: 104  (step: 3)
> > > > Test passed: 104  (step: 4)
> > > > Test passed: 104  (step: 5)
> > > > Test passed: 104  (step: 6)
> > > > Test passed: 104  (step: 7)
> > > > Test passed: 104  (step: 8)
> > > > Test passed: 104  (step: 9)
> > > > Test passed: 104  (step: 10)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 11)
> > > > Test passed: 104  (step: 12)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 13)
> > > > Test passed: 104  (step: 14)
> > > > Test passed: 104  (step: 15)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 16)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 17)
> > > > Test passed: 104  (step: 18)
> > > > Test passed: 104  (step: 19)
> > > > Apache Harmony Launcher : (c) Copyright 1991, 2006 The Apache Software
> > Foundation or its licensors, as applicable.
> > > > java version "1.5.0"
> > > > pre-alpha : not complete or compatible
> > > > svn = r471468, (Nov  7 2006), Linux/ia32/gcc 3.3.3, release build
> > > > http://incubator.apache.org/harmony
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 0)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 1)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 2)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 3)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 4)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 5)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 6)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 7)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 8)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 9)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 10)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 11)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 12)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 13)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 14)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 15)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 16)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 17)
> > > > -9223372032559808513  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 18)
> > > > 9223372032559808512  9223372036854775807  -9223372036854775808
> > > > Test failed: 105  (step: 19)
> > > > This bug is reproducible on Jitrino (JET/OPT) and on interpreter:
> > > > java -cp . volatileTest
> > > > java -cp . -Xint volatileTest
> > > > java -cp . -Xem:opt volatileTest
> > >
> > > --
> > > This message is automatically generated by JIRA.
> > > -
> > > You can reply to this email to add a comment to the issue online.
> > >
> > >
> >
> >
> > --
> > http://xiao-feng.blogspot.com
> >
>
>
>
> --
> Mikhail Fursov
>


-- 
http://xiao-feng.blogspot.com

Mime
View raw message