harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexey Varlamov" <alexey.v.varla...@gmail.com>
Subject Re: [jira] Updated: (HARMONY-2092) [drlvm][jit] Harmony works with volatile variables incorrectly
Date Thu, 24 May 2007 09:48:50 GMT
2007/5/24, Mikhail Fursov <mike.fursov@gmail.com>:
> Xiao-Feng,
> Jitrino on IA32 platform splits every 64-bit integer into two 32-bit values
> and every store/load of 64bit integer is done with 2 separate store/loads of
> 32-bit ones.Before using locks we must have single 64bit store/load
> instruction. When we have it - lock prefix won't be needed at all (AFAIU)
>
> You point about compatibility with no-SSE platform is good.  George, what do
> you think about FISTP to store 64-bit integers to memory?

AFAIR we agreed on SSE as minimal IA32 requirement, and DRLVM uses
sfense instruction for atomic operations support.

>
> On 5/23/07, Xiao-Feng Li <xiaofeng.li@gmail.com> wrote:
> >
> > 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
> >
>
>
>
> --
> Mikhail Fursov
>

Mime
View raw message