From 9a80cbb4174dc338db9584e3fb73d304193bf2b5 Mon Sep 17 00:00:00 2001 From: Colin Surprenant Date: Thu, 24 Sep 2015 15:01:43 -0400 Subject: [PATCH 1/5] move attr_volatile impl into module --- .../ext/SynchronizationLibrary.java | 39 ++++++++------- .../synchronization/jruby_object.rb | 44 +++++++++------- lib/concurrent/synchronization/mri_object.rb | 46 ++++++++++------- lib/concurrent/synchronization/rbx_object.rb | 50 +++++++++++-------- 4 files changed, 103 insertions(+), 76 deletions(-) diff --git a/ext/com/concurrent_ruby/ext/SynchronizationLibrary.java b/ext/com/concurrent_ruby/ext/SynchronizationLibrary.java index ff11da183..9dc8a4a09 100644 --- a/ext/com/concurrent_ruby/ext/SynchronizationLibrary.java +++ b/ext/com/concurrent_ruby/ext/SynchronizationLibrary.java @@ -6,6 +6,7 @@ import org.jruby.RubyClass; import org.jruby.RubyModule; import org.jruby.RubyObject; +import org.jruby.RubyBasicObject; import org.jruby.anno.JRubyClass; import org.jruby.anno.JRubyMethod; import org.jruby.runtime.ObjectAllocator; @@ -47,6 +48,9 @@ public void load(Ruby runtime, boolean wrap) throws IOException { defineModule("Concurrent"). defineModuleUnder("Synchronization"); + RubyModule jrubyAttrVolatileModule = synchronizationModule.defineModuleUnder("JRubyAttrVolatile"); + jrubyAttrVolatileModule.defineAnnotatedMethods(JRubyAttrVolatile.class); + defineClass(runtime, synchronizationModule, "AbstractObject", "JRubyObject", JRubyObject.class, JRUBY_OBJECT_ALLOCATOR); @@ -81,21 +85,12 @@ private RubyClass defineClass(Ruby runtime, RubyModule namespace, String parentN // SynchronizedVariableAccessor wraps with synchronized block, StampedVariableAccessor uses fullFence or // volatilePut - @JRubyClass(name = "JRubyObject", parent = "AbstractObject") - public static class JRubyObject extends RubyObject { + // module JRubyAttrVolatile + public static class JRubyAttrVolatile { private static volatile ThreadContext threadContext = null; - public JRubyObject(Ruby runtime, RubyClass metaClass) { - super(runtime, metaClass); - } - - @JRubyMethod - public IRubyObject initialize(ThreadContext context) { - return this; - } - @JRubyMethod(name = "full_memory_barrier", visibility = Visibility.PRIVATE) - public IRubyObject fullMemoryBarrier(ThreadContext context) { + public static IRubyObject fullMemoryBarrier(ThreadContext context, IRubyObject self) { // Prevent reordering of ivar writes with publication of this instance if (UnsafeHolder.U == null || !UnsafeHolder.SUPPORTS_FENCES) { // Assuming that following volatile read and write is not eliminated it simulates fullFence. @@ -109,35 +104,43 @@ public IRubyObject fullMemoryBarrier(ThreadContext context) { } @JRubyMethod(name = "instance_variable_get_volatile", visibility = Visibility.PROTECTED) - public IRubyObject instanceVariableGetVolatile(ThreadContext context, IRubyObject name) { + public static IRubyObject instanceVariableGetVolatile(ThreadContext context, IRubyObject self, IRubyObject name) { // Ensure we ses latest value with loadFence if (UnsafeHolder.U == null || !UnsafeHolder.SUPPORTS_FENCES) { // piggybacking on volatile read, simulating loadFence final ThreadContext oldContext = threadContext; - return instance_variable_get(context, name); + return ((RubyBasicObject)self).instance_variable_get(context, name); } else { UnsafeHolder.loadFence(); - return instance_variable_get(context, name); + return ((RubyBasicObject)self).instance_variable_get(context, name); } } @JRubyMethod(name = "instance_variable_set_volatile", visibility = Visibility.PROTECTED) - public IRubyObject InstanceVariableSetVolatile(ThreadContext context, IRubyObject name, IRubyObject value) { + public static IRubyObject InstanceVariableSetVolatile(ThreadContext context, IRubyObject self, IRubyObject name, IRubyObject value) { // Ensure we make last update visible if (UnsafeHolder.U == null || !UnsafeHolder.SUPPORTS_FENCES) { // piggybacking on volatile write, simulating storeFence - final IRubyObject result = instance_variable_set(name, value); + final IRubyObject result = ((RubyBasicObject)self).instance_variable_set(name, value); threadContext = context; return result; } else { // JRuby uses StampedVariableAccessor which calls fullFence // so no additional steps needed. // See https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/runtime/ivars/StampedVariableAccessor.java#L151-L159 - return instance_variable_set(name, value); + return ((RubyBasicObject)self).instance_variable_set(name, value); } } } + @JRubyClass(name = "JRubyObject", parent = "AbstractObject") + public static class JRubyObject extends RubyObject { + + public JRubyObject(Ruby runtime, RubyClass metaClass) { + super(runtime, metaClass); + } + } + @JRubyClass(name = "Object", parent = "JRubyObject") public static class Object extends JRubyObject { diff --git a/lib/concurrent/synchronization/jruby_object.rb b/lib/concurrent/synchronization/jruby_object.rb index 38c538c38..91995a140 100644 --- a/lib/concurrent/synchronization/jruby_object.rb +++ b/lib/concurrent/synchronization/jruby_object.rb @@ -3,33 +3,41 @@ module Synchronization if Concurrent.on_jruby? - # @!visibility private - # @!macro internal_implementation_note - class JRubyObject < AbstractObject - - def initialize - # nothing to do + module JRubyAttrVolatile + def self.included(base) + base.extend(ClassMethods) end - def self.attr_volatile(*names) - names.each do |name| + module ClassMethods + def attr_volatile(*names) + names.each do |name| - ivar = :"@volatile_#{name}" + ivar = :"@volatile_#{name}" - class_eval <<-RUBY, __FILE__, __LINE__ + 1 - def #{name} - instance_variable_get_volatile(:#{ivar}) - end + class_eval <<-RUBY, __FILE__, __LINE__ + 1 + def #{name} + instance_variable_get_volatile(:#{ivar}) + end - def #{name}=(value) - instance_variable_set_volatile(:#{ivar}, value) - end - RUBY + def #{name}=(value) + instance_variable_set_volatile(:#{ivar}, value) + end + RUBY + end + names.map { |n| [n, :"#{n}="] }.flatten end - names.map { |n| [n, :"#{n}="] }.flatten end + end + # @!visibility private + # @!macro internal_implementation_note + class JRubyObject < AbstractObject + include JRubyAttrVolatile + + def initialize + # nothing to do + end end end end diff --git a/lib/concurrent/synchronization/mri_object.rb b/lib/concurrent/synchronization/mri_object.rb index 289334034..74202f53e 100644 --- a/lib/concurrent/synchronization/mri_object.rb +++ b/lib/concurrent/synchronization/mri_object.rb @@ -1,35 +1,43 @@ module Concurrent module Synchronization - # @!visibility private - # @!macro internal_implementation_note - class MriObject < AbstractObject + module MriAttrVolatile + def self.included(base) + base.extend(ClassMethods) + end - def initialize - # nothing to do + module ClassMethods + def attr_volatile(*names) + names.each do |name| + ivar = :"@volatile_#{name}" + class_eval <<-RUBY, __FILE__, __LINE__ + 1 + def #{name} + #{ivar} + end + + def #{name}=(value) + #{ivar} = value + end + RUBY + end + names.map { |n| [n, :"#{n}="] }.flatten + end end def full_memory_barrier # relying on undocumented behavior of CRuby, GVL acquire has lock which ensures visibility of ivars # https://github.com/ruby/ruby/blob/ruby_2_2/thread_pthread.c#L204-L211 end + end - def self.attr_volatile(*names) - names.each do |name| - ivar = :"@volatile_#{name}" - class_eval <<-RUBY, __FILE__, __LINE__ + 1 - def #{name} - #{ivar} - end + # @!visibility private + # @!macro internal_implementation_note + class MriObject < AbstractObject + include MriAttrVolatile - def #{name}=(value) - #{ivar} = value - end - RUBY - end - names.map { |n| [n, :"#{n}="] }.flatten + def initialize + # nothing to do end end - end end diff --git a/lib/concurrent/synchronization/rbx_object.rb b/lib/concurrent/synchronization/rbx_object.rb index b6d324d21..396abf988 100644 --- a/lib/concurrent/synchronization/rbx_object.rb +++ b/lib/concurrent/synchronization/rbx_object.rb @@ -1,37 +1,45 @@ module Concurrent module Synchronization + module RbxAttrVolatile + def self.included(base) + base.extend(ClassMethods) + end - # @!visibility private - # @!macro internal_implementation_note - class RbxObject < AbstractObject - def initialize - # nothing to do + module ClassMethods + def attr_volatile(*names) + names.each do |name| + ivar = :"@volatile_#{name}" + class_eval <<-RUBY, __FILE__, __LINE__ + 1 + def #{name} + Rubinius.memory_barrier + #{ivar} + end + + def #{name}=(value) + #{ivar} = value + Rubinius.memory_barrier + end + RUBY + end + names.map { |n| [n, :"#{n}="] }.flatten + end end def full_memory_barrier # Rubinius instance variables are not volatile so we need to insert barrier Rubinius.memory_barrier end + end - def self.attr_volatile *names - names.each do |name| - ivar = :"@volatile_#{name}" - class_eval <<-RUBY, __FILE__, __LINE__ + 1 - def #{name} - Rubinius.memory_barrier - #{ivar} - end + # @!visibility private + # @!macro internal_implementation_note + class RbxObject < AbstractObject + include RbxAttrVolatile - def #{name}=(value) - #{ivar} = value - Rubinius.memory_barrier - end - RUBY - end - names.map { |n| [n, :"#{n}="] }.flatten + def initialize + # nothing to do end end - end end From e46a38f3c77fd75aab6ff691b20dbf65728d49a2 Mon Sep 17 00:00:00 2001 From: Colin Surprenant Date: Thu, 24 Sep 2015 15:02:32 -0400 Subject: [PATCH 2/5] expose Concurrent::Synchronization::Volatile module --- lib/concurrent/synchronization.rb | 1 + lib/concurrent/synchronization/volatile.rb | 35 ++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 lib/concurrent/synchronization/volatile.rb diff --git a/lib/concurrent/synchronization.rb b/lib/concurrent/synchronization.rb index 9c086ffff..32dae59fb 100644 --- a/lib/concurrent/synchronization.rb +++ b/lib/concurrent/synchronization.rb @@ -5,6 +5,7 @@ require 'concurrent/synchronization/jruby_object' require 'concurrent/synchronization/rbx_object' require 'concurrent/synchronization/object' +require 'concurrent/synchronization/volatile' require 'concurrent/synchronization/abstract_lockable_object' require 'concurrent/synchronization/mri_lockable_object' diff --git a/lib/concurrent/synchronization/volatile.rb b/lib/concurrent/synchronization/volatile.rb new file mode 100644 index 000000000..12915aaf2 --- /dev/null +++ b/lib/concurrent/synchronization/volatile.rb @@ -0,0 +1,35 @@ +module Concurrent + module Synchronization + + # Volatile adds the attr_volatile class method when included. + # + # @example + # class Foo + # include Concurrent::Synchronization::Volatile + # + # attr_volatile :bar + # + # def initialize + # @volatile_bar = 1 + # end + # end + # + # foo = Foo.new + # foo.bar + # => 1 + # foo.bar = 2 + # => 2 + + Volatile = case + when Concurrent.on_cruby? + MriAttrVolatile + when Concurrent.on_jruby? + JRubyAttrVolatile + when Concurrent.on_rbx? + RbxAttrVolatile + else + warn 'Possibly unsupported Ruby implementation' + MriAttrVolatile + end + end +end \ No newline at end of file From fcfae7f2fabc1558f8afc35eb317a0e379c81433 Mon Sep 17 00:00:00 2001 From: Colin Surprenant Date: Thu, 24 Sep 2015 19:19:10 -0400 Subject: [PATCH 3/5] comment voaltile threadContext --- ext/com/concurrent_ruby/ext/SynchronizationLibrary.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ext/com/concurrent_ruby/ext/SynchronizationLibrary.java b/ext/com/concurrent_ruby/ext/SynchronizationLibrary.java index 9dc8a4a09..b1aa584df 100644 --- a/ext/com/concurrent_ruby/ext/SynchronizationLibrary.java +++ b/ext/com/concurrent_ruby/ext/SynchronizationLibrary.java @@ -87,6 +87,10 @@ private RubyClass defineClass(Ruby runtime, RubyModule namespace, String parentN // module JRubyAttrVolatile public static class JRubyAttrVolatile { + + // volatile threadContext is used as a memory barrier per the JVM memory model happens-before semantic + // on volatile fields. any volatile field could have been used but using the thread context is an + // attempt to avoid code elimination. private static volatile ThreadContext threadContext = null; @JRubyMethod(name = "full_memory_barrier", visibility = Visibility.PRIVATE) From 69669ea76d969635a2a2ab45f18fda7695777a1e Mon Sep 17 00:00:00 2001 From: Colin Surprenant Date: Fri, 25 Sep 2015 15:41:32 -0400 Subject: [PATCH 4/5] extract attr_volatile as a shared_examples --- spec/concurrent/synchronization_spec.rb | 74 +++++++++++++++++-------- 1 file changed, 50 insertions(+), 24 deletions(-) diff --git a/spec/concurrent/synchronization_spec.rb b/spec/concurrent/synchronization_spec.rb index 2520f192f..89b6b16de 100644 --- a/spec/concurrent/synchronization_spec.rb +++ b/spec/concurrent/synchronization_spec.rb @@ -3,6 +3,36 @@ module Concurrent describe Synchronization do + + shared_examples :attr_volatile do + + specify 'older writes are always visible' do + # store = BClass.new + store.not_volatile = 0 + store.volatile = 0 + + t1 = Thread.new do + Thread.abort_on_exception = true + 1000000000.times do |i| + store.not_volatile = i + store.volatile = i + end + end + + t2 = Thread.new do + 10.times do + volatile = store.volatile + not_volatile = store.not_volatile + expect(not_volatile).to be >= volatile + Thread.pass + end + end + + t2.join + t1.kill + end + end + describe Synchronization::Object do class AAClass < Synchronization::Object end @@ -40,6 +70,14 @@ class ADClass < ACClass # TODO (pitr 12-Sep-2015): give a whole gem a pass to find classes with final fields without using the convention and migrate Synchronization::Object.ensure_safe_initialization_when_final_fields_are_present + + class VolatileFieldClass < Synchronization::Object + attr_volatile :volatile + attr_accessor :not_volatile + end + + let(:store) { VolatileFieldClass.new } + it_should_behave_like :attr_volatile end describe Synchronization::LockableObject do @@ -142,33 +180,21 @@ def ns_initialize t1.kill end - describe 'attr volatile' do - specify 'older writes are always visible' do - store = BClass.new - store.not_volatile = 0 - store.volatile = 0 - - t1 = Thread.new do - Thread.abort_on_exception = true - 1000000000.times do |i| - store.not_volatile = i - store.volatile = i - end - end + let(:store) { BClass.new } + it_should_behave_like :attr_volatile + end - t2 = Thread.new do - 10.times do - volatile = store.volatile - not_volatile = store.not_volatile - expect(not_volatile).to be >= volatile - Thread.pass - end - end + describe 'Concurrent::Synchronization::Volatile module' do + class BareClass + include Synchronization::Volatile - t2.join - t1.kill - end + attr_volatile :volatile + attr_accessor :not_volatile end + + let(:store) { BareClass.new } + it_should_behave_like :attr_volatile end + end end From a4a9a769d712af594d1eb82dc1f86f27eca89d44 Mon Sep 17 00:00:00 2001 From: Colin Surprenant Date: Fri, 25 Sep 2015 15:47:12 -0400 Subject: [PATCH 5/5] correct example to use volatile method --- lib/concurrent/synchronization/volatile.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/concurrent/synchronization/volatile.rb b/lib/concurrent/synchronization/volatile.rb index 12915aaf2..07f563910 100644 --- a/lib/concurrent/synchronization/volatile.rb +++ b/lib/concurrent/synchronization/volatile.rb @@ -10,7 +10,7 @@ module Synchronization # attr_volatile :bar # # def initialize - # @volatile_bar = 1 + # self.bar = 1 # end # end #