Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Concurrency classpath extension (part of the atomic package implementation) #149

Merged
merged 11 commits into from Jan 4, 2014

Conversation

Projects
None yet
3 participants
Contributor

jentfoo commented Jan 3, 2014

Here is an implementation for AtomicBoolean, AtomicInteger, AtomicLong, and AtomicReference. Thanks to Joel for doing the hard work, these were all pretty straight forward to implement.

The unit test basic confirm that compare and swap is being used correctly, and works correctly. I verified it with much higher numbers, but reduced them to keep unit tests fast.

@dicej dicej and 1 other commented on an outdated diff Jan 3, 2014

classpath/java/util/concurrent/atomic/AtomicBoolean.java
+ private final AtomicInteger value;
+
+ public AtomicBoolean() {
+ this(false);
+ }
+
+ public AtomicBoolean(boolean initialValue) {
+ value = new AtomicInteger(intValue(initialValue));
+ }
+
+ private static int intValue(boolean value) {
+ return value ? TRUE_VALUE : FALSE_VALUE;
+ }
+
+ private static boolean booleanValue(int value) {
+ if (value == TRUE_VALUE) {
@dicej

dicej Jan 3, 2014

Member

Why not just "return value == TRUE_VALUE;"?

@jentfoo

jentfoo Jan 3, 2014

Contributor

Sure, I can make that change if you prefer. I originally was throwing an exception if it was neither, but decided the exception was not worth it.

@dicej

dicej Jan 3, 2014

Member

Don't worry about changing it; I just seemed a little odd, but the exception thing makes sense.

@dicej dicej and 2 others commented on an outdated diff Jan 3, 2014

test/AtomicIntegerTest.java
+ new Thread(new Runnable() {
+ @Override
+ public void run() {
+ try {
+ doOperation();
+ waitTillReady();
+ } finally {
+ synchronized (threadDoneCount) {
+ threadDoneCount.incrementAndGet();
+
+ threadDoneCount.notifyAll();
+ }
+ }
+ }
+
+ private void waitTillReady() {
@dicej

dicej Jan 3, 2014

Member

I don't understand the purpose of waitTillReady.

@jentfoo

jentfoo Jan 3, 2014

Contributor

The goal is to increase the chances of thread interaction. It likely will take longer to create the threads, than it will to actually run the doOperation loop. So we buffer how long each thread should wait (adjusted based off actual time). That way hopefully all the threads will start the doOperation() point at close to the same time, and that way the Atomic(Whatever) will have to deal with compare and swap failures.

@joshuawarner32

joshuawarner32 Jan 3, 2014

Contributor

@jentfoo, along that line of thought, have you tried incorrectly implementing Atomic* (i.e. with non-atomic operations), and then run these tests? I just want to make sure the tests will actually give us some information...

@dicej

dicej Jan 3, 2014

Member

Why does waitTillReady come after doOperation? It doesn't seem like that will affect the degree of concurrency with which the threads run doOperation; only when they exit, which doesn't matter.

@jentfoo

jentfoo Jan 3, 2014

Contributor

Whoops, that was a mistake..I will correct that now.

@joshuawarner32 I have not, but I do know from experience that if you use a volatile in replace of the atomic implementation that the check then write failure will occur. Causing some of the increments or decrements to fail. Of course that failure is not guaranteed, but the more threads and the longer they loop it becomes very likely. Which is why I personally tested this with much higher values than I actually committed in.

@joshuawarner32

joshuawarner32 Jan 3, 2014

Contributor

I understand that, given a proper test, it would find such problems pretty quickly. What I'm more concerned about is making sure the test doesn't have any (more) systematic errors like the waitTillReady one you've been discussing, which might make it highly unlikely for the test to fail, even with such a "volatile" implementation.

@dicej

dicej Jan 3, 2014

Member

I would prefer to see a version of waitTillReady that synchronizes and waits until the main thread does a notifyAll to set them all loose.

@dicej

dicej Jan 3, 2014

Member

Did I ever mention I hate Thread.sleep? Hate, hate, hate, hate it.

Contributor

jentfoo commented Jan 3, 2014

I went ahead and pushed the change to do a wait/notify in the test compared to the .sleep().

Member

dicej commented Jan 3, 2014

Looks better, thanks. I wonder if it would be practical to combine all three tests into a single file and pass in doOperation and the verification code as two runnables to a single method which runs the test. That way we don't have to duplicate so much code.

Contributor

joshuawarner32 commented Jan 3, 2014

FWIW, I tried implementing AtomicInteger.compareAndSet with non-atomic operations - and your AtomicInteger test failed 10/10 times. LGTM.

Contributor

jentfoo commented Jan 3, 2014

Nice, thanks josh. I am working to combine these into a single class now.

Member

dicej commented Jan 4, 2014

Thanks, Mike. Looks great.

dicej added a commit that referenced this pull request Jan 4, 2014

Merge pull request #149 from jentfoo/concurrency_classpath_extension
Concurrency classpath extension (part of the atomic package implementation)

@dicej dicej merged commit 1f6051b into ReadyTalk:master Jan 4, 2014

1 check passed

default The Travis CI build passed
Details

dicej added a commit to dicej/avian that referenced this pull request Apr 21, 2014

Merge pull request #149 from jentfoo/concurrency_classpath_extension
Concurrency classpath extension (part of the atomic package implementation)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment