Skip to content

Conversation

lucasallan
Copy link
Member

@jdantonio @pitr-ch Could you guys review this pull request?

@lucasallan lucasallan force-pushed the java-extension-for-atomic branch 3 times, most recently from f547d76 to 4523c40 Compare February 10, 2015 06:58
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should return result of compareAndSet rather than always nil.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some rules around it. I got some falling specs so now I'm fixing them. Thanks for pointing it out.

@lucasallan lucasallan force-pushed the java-extension-for-atomic branch 2 times, most recently from fd794d9 to 39ca9db Compare February 10, 2015 18:47
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 91.73% when pulling fd794d9 on java-extension-for-atomic into 5e2ae69 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.12%) to 91.65% when pulling 39ca9db on java-extension-for-atomic into 5e2ae69 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.34%) to 91.43% when pulling ceeae16 on java-extension-for-atomic into 5e2ae69 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.34%) to 91.43% when pulling ceeae16 on java-extension-for-atomic into 5e2ae69 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 91.69% when pulling ceeae16 on java-extension-for-atomic into 5e2ae69 on master.

lucasallan added a commit that referenced this pull request Feb 10, 2015
 [Improvement] - Initial implementation of Concurrent::AtomicBoolean in pure Java
@lucasallan lucasallan merged commit 3964a7b into master Feb 10, 2015
@jdantonio
Copy link
Member

@lucasallan Did you have an opportunity to run performance tests to verify that this approach is faster?

@lucasallan
Copy link
Member Author

@jdantonio yes, I did and looks good (similar to the results we got on ref).
I'll post the results asap - I'm using my personal computer now and I don't have them.

@lucasallan
Copy link
Member Author

But as you can see in this preview:

Java Version

~~~ JRuby version: 1.7.19
       user     system      total        real
Benchmarking Concurrent::AtomicFixnum#up...
   7.760000   0.050000   7.810000 (  7.375000)
Benchmarking Concurrent::AtomicFixnum#down...
   7.280000   0.030000   7.310000 (  7.275000)
Benchmarking Concurrent::AtomicFixnum#value...
   7.150000   0.030000   7.180000 (  7.154000)

JRuby Version

~~~ JRuby version: 1.7.19
       user     system      total        real
Benchmarking Concurrent::AtomicFixnum#up...
  24.120000   0.110000  24.230000 ( 23.738000)
Benchmarking Concurrent::AtomicFixnum#down...
  23.390000   0.070000  23.460000 ( 23.427000)
Benchmarking Concurrent::AtomicFixnum#value...
  21.390000   0.120000  21.510000 ( 21.655000)

Ruby code:

require 'concurrent'
require 'benchmark'

NUM = 50_000_000

if defined? JRUBY_VERSION
  puts "~~~ JRuby version: #{JRUBY_VERSION}"

  Benchmark.bm do |stats|
    e = Concurrent::AtomicFixnum.new()

    puts "Benchmarking Concurrent::AtomicFixnum#up..."
    stats.report do
      NUM.times { e.up }
    end

    puts "Benchmarking Concurrent::AtomicFixnum#down..."
    stats.report do
      NUM.times { e.down }
    end

    puts "Benchmarking Concurrent::AtomicFixnum#value..."
    stats.report do
      NUM.times { e.value }
    end
  end

end

@jdantonio
Copy link
Member

Awesome! Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could also be getRuntime().newBoolean(atomicBoolean.compareAndSet(false, true)).

@lucasallan lucasallan deleted the java-extension-for-atomic branch February 11, 2015 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants