Synchronization ExecutorService #283
Conversation
|
@pitr-ch This PR incorporates your suggestions. I quickly made the updates before work this morning so I expect I probably missed much. |
|
@jdantonio Should I review or will you be working on it more first? |
|
@pitr-ch I consider this a spike and not ready to merge, but please provide your thoughts. I will be working on it more this evening. I prefer this approach over the other two spikes but we should explicitly choose one of the three approaches. I also want to incorporate the updates @headius suggested in #280 and incorporate #284 once it's been merged to master. I was unable to fully review #284 last night (I gave a presentation to a small group of students at the University of Akron) but I plan to review it more fully today. My initial impression is that #284 should go into master first. |
| synchronize do | ||
| @stop_event = Event.new | ||
| @stopped_event = Event.new | ||
| end |
There was a problem hiding this comment.
if you wait for #284 you can use ns_initialize here.
There was a problem hiding this comment.
Correction you can consider those two fields final so:
def initialize
super()
@StopEvent = Event.new
@StoppedEvent = Event.new
ensure_ivar_visibility!
end5d8dc0f to
3c32888
Compare
|
The last update implements the performance improvements from #280. The performance difference is significant: Before: After: |
| # is not running | ||
| # | ||
| # @raise [ArgumentError] if no task is given | ||
| def post(*args, &task) |
There was a problem hiding this comment.
Out of curiosity, what kind of args would you imagine being sent to a task?
There was a problem hiding this comment.
Any values needed within the task can be passed via args. Ruby has closures so any values needed within the task can be accessed via the enclosing context. Doing so, however, requires the runtime to retain the entire closure. This may be inefficient if the closure is large and only a few values are needed. Passing data as arguments instead will allow the runtime to release the closure.
There was a problem hiding this comment.
Ah yes that's what I was imagining. Good call. We should document this behavior somewhere as well.
There was a problem hiding this comment.
There is also another danger. The access to closure's variables is not synchronized (on JRuby, based on documentation) which may lead to issues since the task is executed on different thread. The args should be protected against that to avoid this issue, e.g. Job object with final fields.
@jdantonio do you know about any sources talking about the closure releasing? I would like to know more. I always assumed that the closure is held because of following situation for example:
[1] pry(main)> a = 1
=> 1
[2] pry(main)> proc { local_variables }
=> #<Proc:0x007f21885d3850@(pry):2>
[4] pry(main)> proc { |s| eval s }.call 'local_variables'
=> [:s, :a, :__, :_, :_dir_, :_file_, :_ex_, :_pry_, :_out_, :_in_]There was a problem hiding this comment.
@pitr-ch The issue of closures came up a long time ago in a conversation another PR or Issue. Someone had written a (very favorable) blog post in which it was noted that arguments couldn't be passed into one of our abstractions. I can't remember which one (I want to say it was Future, but I honestly can't remember). At issue was relying on the closure vs. a scoped variable. I don't remember the details. The result of the conversation was that I added a feature to allow argument passing. I'll see if I can find the Issue/PR.
JRuby Executor PerformanceSee #280 Old Implementationjruby-1.7.18jruby-headNew Implementationjruby-1.7.18jruby-head |
|
I believe this PR is ready to merge. The failing test is the |
| @@ -55,14 +55,14 @@ public IRubyObject initialize(ThreadContext context, IRubyObject[] args, Block b | |||
| } | |||
| } | |||
|
|
|||
| @JRubyMethod(name = "synchronize", visibility = Visibility.PRIVATE) | |||
| @JRubyMethod(name = "synchronize", visibility = Visibility.PROTECTED) | |||
There was a problem hiding this comment.
Just curious what is behind this change?
There was a problem hiding this comment.
With those methods private the subclasses didn't work. Every call to synchronize on JRuby raised an exception.
There was a problem hiding this comment.
Can you show me the synchronize calls that were failing? Sounds like we may have a visibility-checking bug somewhere.
There was a problem hiding this comment.
@headius Change line 58 to Visibility.PRIVATE and run the specs. Or simply load up IRB and use any class that subclasses Synchronization::Object. You will get NoMethodError: private methodsynchronize' called for...any time a subclass calls the#synchronizemethod. Which, from where I'm sitting, is the expected behavior. Private visibility should prevent subclasses from calling a method on a parent whereas protected visibility allows subclasses to access the method. TheSynchronization::Objectclass [extends](https://github.com/ruby-concurrency/concurrent-ruby/blob/master/lib/concurrent/synchronization/object.rb) theSynchronization::JavaObject` class so protected is the correct visibility--given the way we structure things internally.
Since Delay is synchronized and we use it to lazy-load the global thread pools, you'll never get the gem to load if you change line 58 above. Commit 0181005 is a quick-and-dirty update which changes the visibility of JavaObject and changes the visibility of synchronize back to protected withinDelay. On that branch if you try to create anIVarwithinbundle console` you'll get this:
jruby-1.7.19 :001 > require 'concurrent'
=> true
jruby-1.7.19 :002 > ivar = Concurrent::IVar.new
NoMethodError: private method `synchronize' called for #<Concurrent::IVar:0x490caf5f>
from /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/dereferenceable.rb:87:in `set_deref_options'
from /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/ivar.rb:154:in `ns_initialize'
from com/concurrent_ruby/ext/SynchronizationLibrary.java:54:in `initialize'
from /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/ivar.rb:64:in `initialize'
from (irb):2:in `evaluate'
from org/jruby/RubyKernel.java:1107:in `eval'
from org/jruby/RubyKernel.java:1507:in `loop'
from org/jruby/RubyKernel.java:1270:in `catch'
from org/jruby/RubyKernel.java:1270:in `catch'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/lib/bundler/cli/console.rb:14:in `run'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/lib/bundler/cli.rb:300:in `console'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/lib/bundler/vendor/thor/lib/thor.rb:359:in `dispatch'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/lib/bundler/vendor/thor/lib/thor/base.rb:440:in `start'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/lib/bundler/cli.rb:10:in `start'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/bin/bundle:20:in `(root)'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/lib/bundler/friendly_errors.rb:7:in `with_friendly_errors'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/bundler-1.9.6/bin/bundle:18:in `(root)'
from org/jruby/RubyKernel.java:1087:in `load'
from /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/bin/bundle:23:in `(root)'jruby-1.7.19 :003 >
jruby-1.7.19 :004 >
On master if you run the specs you get this:
$ rspec spec/concurrent/delay_spec.rb
/Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/simplecov-0.9.2/lib/simplecov.rb:31 warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
NoMethodError: private method `synchronize' called for #<Concurrent::Delay:0x4abf3f0>
set_deref_options at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/dereferenceable.rb:87
ns_initialize at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/delay.rb:167
initialize at com/concurrent_ruby/ext/SynchronizationLibrary.java:54
initialize at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/delay.rb:72
initialize at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/utility/processor_count.rb:8
Concurrent at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/utility/processor_count.rb:143
(root) at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/utility/processor_count.rb:4
require at org/jruby/RubyKernel.java:1071
require at /Users/jdantonio/.rvm/rubies/jruby-1.7.19/lib/ruby/shared/rubygems/core_ext/kernel_require.rb:54
(root) at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/configuration.rb:1
require at org/jruby/RubyKernel.java:1071
require at /Users/jdantonio/.rvm/rubies/jruby-1.7.19/lib/ruby/shared/rubygems/core_ext/kernel_require.rb:54
(root) at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent/configuration.rb:6
require at org/jruby/RubyKernel.java:1071
require at /Users/jdantonio/.rvm/rubies/jruby-1.7.19/lib/ruby/shared/rubygems/core_ext/kernel_require.rb:54
(root) at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent.rb:1
require at org/jruby/RubyKernel.java:1071
(root) at /Users/jdantonio/Projects/opensource/concurrent-ruby/lib/concurrent.rb:6
(root) at /Users/jdantonio/Projects/opensource/concurrent-ruby/spec/spec_helper.rb:1
each at org/jruby/RubyArray.java:1613
(root) at /Users/jdantonio/Projects/opensource/concurrent-ruby/spec/spec_helper.rb:23
(root) at /Users/jdantonio/.rvm/rubies/jruby-1.7.19/lib/ruby/shared/rubygems/core_ext/kernel_require.rb:1
each at org/jruby/RubyArray.java:1613
require at /Users/jdantonio/.rvm/rubies/jruby-1.7.19/lib/ruby/shared/rubygems/core_ext/kernel_require.rb:54
requires= at /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/rspec-core-3.2.3/lib/rspec/core/configuration.rb:1181
requires= at /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/rspec-core-3.2.3/lib/rspec/core/configuration.rb:1181
process_options_into at /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/rspec-core-3.2.3/lib/rspec/core/configuration_options.rb:110
process_options_into at /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/rspec-core-3.2.3/lib/rspec/core/configuration_options.rb:109
configure at /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/rspec-core-3.2.3/lib/rspec/core/configuration_options.rb:22
setup at /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/gems/rspec-core-3.2.3/lib/rspec/core/runner.rb:96
load at org/jruby/RubyKernel.java:1087
(root) at /Users/jdantonio/.rvm/gems/jruby-1.7.19@concurrent-ruby/bin/rspec:23
There was a problem hiding this comment.
@jdantonio it's counter intuitive but private methods can be actually called from sub-classes in Ruby
A = Class.new { private def a; :a; end }; B = Class.new(A); B.new.instance_eval {a} # => :aIt seemed weird to me too, I should have thought about it more.
There was a problem hiding this comment.
The calls you point out are both from within a module, calling synchronize against @mutex, which is presumably the IVar above. I suspect JRuby has a problem with visibility from module methods. Thanks for the info.
There was a problem hiding this comment.
Actually I am not sure about this code now. I was unable to get a module to dispatch to a private method even on a hierarchy that had included it. Further, I was unable to get a dotted form (foo.bar) to dispatch to a parent's private methods. Only the bare version worked:
[] ~/projects/jruby $ rvm ruby-2.2 do ruby -e "class A; private def bar; end; end; class B < A; def foo; bar; end; end; B.new.foo"
[] ~/projects/jruby $ rvm ruby-2.2 do ruby -e "class A; private def bar; end; end; class B < A; def foo; self.bar; end; end; B.new.foo"
-e:1:in `foo': private method `bar' called for #<B:0x007fc681927300> (NoMethodError)
from -e:1:in `<main>'
[] ~/projects/jruby $ rvm ruby-2.2 do ruby -e "class A; private def bar; end; end; class B < A; def foo; (a = self; a).bar; end; end; B.new.foo"
-e:1:in `foo': private method `bar' called for #<B:0x007fe82a0df218> (NoMethodError)
from -e:1:in `<main>'
There was a problem hiding this comment.
@pitr-ch All these years using Ruby and I never realize that subclasses could access private methods of superclasses--simply because I've never tried it. Your phrase "counter intuitive" is very polite. I like the word "dumb" better. That's not what protected is supposed to mean! I'll bring that up with Matz the next time he and I chat. :-) Seriously, though, I had to write some code and run it myself. Sadly, it worked exactly as you described...
@headius Ruby is as Ruby does, I guess. Thanks for taking the time to look into it but for now we'll just keep it protected. Behavior aside, I think protected is semantically and logically correct for what we're doing.
|
👍 |
…r-service Synchronization ExecutorService
Creates a new
ExecutorServiceand related series of modules and classes through which all executors use the new synchronization. Additionally:PerThreadExecutortoSimpleExecutorServicens_initializeIVarto better support subclasses usingns_initialize