Fix write loss on concurrent ivar table growth #479

Closed
wants to merge 19 commits into
from

Conversation

Projects
None yet
3 participants
@the8472
Contributor

the8472 commented Jan 5, 2013

Fixes jruby/jruby#476

Under java8 an inlined and stripped-down version of stampedlock with fences is used, java6/7 use a stamped lock with volatile writes and JVMs that do not support sun.misc.Unsafe will use synchronized blocks for writes.

Note that this code requires oracle java 1.8 to build against for the sun.misc.Unsafe class. I added a stub class in the build_lib dir but that has to be integrated in the build process.
Checks whether the necessary methods are actually available are performed at runtime.

Tested with

Java(TM) SE Runtime Environment (build 1.8.0-ea-b70)
Java HotSpot(TM) 64-Bit Server VM (build 25.0-b14, mixed mode)
@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Jan 6, 2013

Owner

I strongly object to relying on Java 8 functionality, primarily because it is not released yet. If it doesn't compile with Java 6, we cannot merge this.

Owner

BanzaiMan commented Jan 6, 2013

I strongly object to relying on Java 8 functionality, primarily because it is not released yet. If it doesn't compile with Java 6, we cannot merge this.

@the8472

This comment has been minimized.

Show comment Hide comment
@the8472

the8472 Jan 6, 2013

Contributor

Did you even read my initial comment?

Under java8 an inlined and stripped-down version of stampedlock with fences is used, java6/7 use a stamped lock with volatile writes and JVMs that do not support sun.misc.Unsafe will use synchronized blocks for writes.

Contributor

the8472 commented Jan 6, 2013

Did you even read my initial comment?

Under java8 an inlined and stripped-down version of stampedlock with fences is used, java6/7 use a stamped lock with volatile writes and JVMs that do not support sun.misc.Unsafe will use synchronized blocks for writes.

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Jan 6, 2013

Owner

I did. Travis build is broken, so I assumed that this pull request does not work on Java 6. Am I mistaken?

Owner

BanzaiMan commented Jan 6, 2013

I did. Travis build is broken, so I assumed that this pull request does not work on Java 6. Am I mistaken?

@the8472

This comment has been minimized.

Show comment Hide comment
@the8472

the8472 Jan 6, 2013

Contributor

Correct, that's why I said the stub class needs to be integrated into the build process.

Anyway, I'm talking with @headius about a solution to that.

Contributor

the8472 commented Jan 6, 2013

Correct, that's why I said the stub class needs to be integrated into the build process.

Anyway, I'm talking with @headius about a solution to that.

@ghost ghost assigned headius Jan 6, 2013

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Jan 6, 2013

Owner

OK. Sounds good.

Owner

BanzaiMan commented Jan 6, 2013

OK. Sounds good.

@headius

This comment has been minimized.

Show comment Hide comment
@headius

headius Jan 6, 2013

Owner

I have pushed licm-opts that includes all above changes plus incorporation of Java 8's Unsafe to allow building on 6 and 7. There's an outstanding issue, though, in that the new impl does not yet avoid the lost writes it was originally intended to fix. @the8472 is continuing to explore that.

Owner

headius commented Jan 6, 2013

I have pushed licm-opts that includes all above changes plus incorporation of Java 8's Unsafe to allow building on 6 and 7. There's an outstanding issue, though, in that the new impl does not yet avoid the lost writes it was originally intended to fix. @the8472 is continuing to explore that.

@the8472

This comment has been minimized.

Show comment Hide comment
@the8472

the8472 Jan 6, 2013

Contributor

Build should be passing with headius' change. The latest failure seems to be an intermittent issue with an unrelated test.

Contributor

the8472 commented Jan 6, 2013

Build should be passing with headius' change. The latest failure seems to be an intermittent issue with an unrelated test.

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Jan 7, 2013

Owner

Reran the build, which succeeded.

Owner

BanzaiMan commented Jan 7, 2013

Reran the build, which succeeded.

@headius

This comment has been minimized.

Show comment Hide comment
@headius

headius Jan 7, 2013

Owner

Merged as a squashed commit with a longer description of the changes in 0e612db. Thanks much for the research and development of this, @the8472 :-)

Owner

headius commented Jan 7, 2013

Merged as a squashed commit with a longer description of the changes in 0e612db. Thanks much for the research and development of this, @the8472 :-)

@headius headius closed this Jan 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment