-
Notifications
You must be signed in to change notification settings - Fork 266
Lambda in main thread and Enumerator support #356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 4 commits
36af69b
28b74f9
5d5eb63
9d9bd11
756685f
60ff3ac
4823fcc
3eda109
7e8a77c
20cb415
7e76725
5eea888
ec0083b
074eab7
70208e6
98a74e4
f3a9dd2
9b9f4bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -97,20 +97,26 @@ def wait | |||||||||
|
|
||||||||||
| class JobFactory | ||||||||||
| def initialize(source, mutex) | ||||||||||
| @lambda = (source.respond_to?(:call) && source) || queue_wrapper(source) | ||||||||||
| @source = source.to_a unless @lambda # turn Range and other Enumerable-s into an Array | ||||||||||
| @lambda = enum_wrapper(source) || (source.respond_to?(:call) && source) || queue_wrapper(source) | ||||||||||
| @source = source.to_a unless @lambda # turn non-Enumerable-s into an Array | ||||||||||
| @runloop_queue = Thread::Queue.new if @lambda | ||||||||||
| @mutex = mutex | ||||||||||
| @index = -1 | ||||||||||
| @stopped = false | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def next | ||||||||||
| if producer? | ||||||||||
| def next(queue_for_thread = nil) | ||||||||||
| if @runloop_queue && queue_for_thread | ||||||||||
| return if @stopped | ||||||||||
| item = runloop_enq(queue_for_thread) | ||||||||||
| return if item == Stop | ||||||||||
|
Comment on lines
+114
to
+116
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment says that we don't need to check for stop but then we check for stop ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This "check for stop" means assigning |
||||||||||
| index = @index += 1 | ||||||||||
| elsif producer? | ||||||||||
| # - index and item stay in sync | ||||||||||
| # - do not call lambda after it has returned Stop | ||||||||||
| item, index = @mutex.synchronize do | ||||||||||
| return if @stopped | ||||||||||
| item = @lambda.call | ||||||||||
| item = call_lambda | ||||||||||
| @stopped = (item == Stop) | ||||||||||
| return if @stopped | ||||||||||
| [item, @index += 1] | ||||||||||
|
|
@@ -123,6 +129,25 @@ def next | |||||||||
| [item, index] | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def runloop | ||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would this make sense ?
Suggested change
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. some method comments would help here too, what does it do exactly
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. Please feel free to change anything.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
does this work ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed the comment says " |
||||||||||
| return unless @runloop_queue | ||||||||||
|
|
||||||||||
| loop do | ||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this right ?
Suggested change
|
||||||||||
| queue = @runloop_queue.pop | ||||||||||
| return if queue == Stop | ||||||||||
| item = call_lambda | ||||||||||
| queue.push(item) | ||||||||||
| break if item == Stop | ||||||||||
| end | ||||||||||
| @stopped = true | ||||||||||
| begin | ||||||||||
|
takahiro-blab marked this conversation as resolved.
|
||||||||||
| while queue = @runloop_queue.pop(true) | ||||||||||
| queue.push(Stop) if queue != Stop # Unlock waiting threads. | ||||||||||
| end | ||||||||||
| rescue ThreadError # All threads are unlocked. | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def size | ||||||||||
| if producer? | ||||||||||
| Float::INFINITY | ||||||||||
|
|
@@ -142,15 +167,33 @@ def unpack(data) | |||||||||
| producer? ? data : [@source[data], data] | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def stopper = @runloop_queue&.push(Stop) | ||||||||||
|
takahiro-blab marked this conversation as resolved.
Outdated
|
||||||||||
|
|
||||||||||
| private | ||||||||||
|
|
||||||||||
| def call_lambda | ||||||||||
| @lambda.call | ||||||||||
| rescue StopIteration | ||||||||||
| Stop | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def runloop_enq(queue_for_thread) | ||||||||||
| @runloop_queue.push(queue_for_thread) | ||||||||||
| queue_for_thread.pop # Wait until @lambda returns. | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def producer? | ||||||||||
| @lambda | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def queue_wrapper(array) | ||||||||||
| array.respond_to?(:num_waiting) && array.respond_to?(:pop) && -> { array.pop(false) } | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def enum_wrapper(source) | ||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe give some examples of what types this is trying to detect
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are asking about This method aims converting So, as first, checking For example: enum_wrapper([1,2,3]) # -> false
enum_wrapper(1..5) # -> Method ( (1..5).method(:next) )
enum_wrapper(Prime.to_enum) # -> Method (See infinite_sequece.rb test case)
takahiro-blab marked this conversation as resolved.
|
||||||||||
| # Convert what is inaccessible by the index | ||||||||||
| !source.respond_to?(:[]) && source.respond_to?(:next) && source.method(:next) | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
| class UserInterruptHandler | ||||||||||
|
|
@@ -211,13 +254,24 @@ def restore_interrupt(old, signal) | |||||||||
| class << self | ||||||||||
| def in_threads(options = { count: 2 }) | ||||||||||
| threads = [] | ||||||||||
| count, = extract_count_from_options(options) | ||||||||||
| count, options = extract_count_from_options(options) | ||||||||||
| finished_monitor = options[:runloop] && Queue.new(1..(count - 1)) # Insert values, one less in count than the number of threads. | ||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. explain why 1 less
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's because the last thread must raise For example, if there are 5 worker threads (when In rescue section, the last one thread will call
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah got it, very complicated ... can you leave some short inline comment to explain a bit
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added comment to this code with below question. |
||||||||||
| stopper = options[:stopper] | ||||||||||
|
|
||||||||||
| Thread.handle_interrupt(Exception => :never) do | ||||||||||
| Thread.handle_interrupt(Exception => :immediate) do | ||||||||||
| count.times do |i| | ||||||||||
| threads << Thread.new { yield(i) } | ||||||||||
| threads << Thread.new do | ||||||||||
| yield(i) | ||||||||||
| ensure | ||||||||||
| begin | ||||||||||
| finished_monitor&.pop(true) # This must be executed even if the worker thread is killed (by #work_in_processes). | ||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. explain why it needs to be executed
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is not case, ( And, this logic is also necessary for terminating operations by Ctrl+C or workers' throwing
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thx, can you leave a bit of this inline for future archeologists :)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added comment with above question. |
||||||||||
| rescue ThreadError # Queue#pop raises ThreadError when the queue is empty. | ||||||||||
| stopper&.call # Stop JobFactory#runloop | ||||||||||
| end | ||||||||||
| end | ||||||||||
| end | ||||||||||
| options[:runloop]&.call # Invoke lambda in caller thread, and provide jobs to thread queue. | ||||||||||
| threads.map(&:value) | ||||||||||
| end | ||||||||||
| ensure | ||||||||||
|
|
@@ -431,10 +485,11 @@ def work_in_threads(job_factory, options, &block) | |||||||||
| results_mutex = Mutex.new # arrays are not thread-safe on jRuby | ||||||||||
| exception = nil | ||||||||||
|
|
||||||||||
| in_threads(options) do |worker_num| | ||||||||||
| in_threads(options.merge(runloop: job_factory.method(:runloop), stopper: job_factory.method(:stopper))) do |worker_num| | ||||||||||
|
takahiro-blab marked this conversation as resolved.
Outdated
|
||||||||||
| queue_for_thread = Thread::Queue.new | ||||||||||
| self.worker_number = worker_num | ||||||||||
| # as long as there are more jobs, work on one of them | ||||||||||
| while !exception && (set = job_factory.next) | ||||||||||
| while !exception && (set = job_factory.next(queue_for_thread)) | ||||||||||
| begin | ||||||||||
| item, index = set | ||||||||||
| result = with_instrumentation item, index, options do | ||||||||||
|
|
@@ -523,15 +578,16 @@ def work_in_processes(job_factory, options, &blk) | |||||||||
| exception = nil | ||||||||||
|
|
||||||||||
| UserInterruptHandler.kill_on_ctrl_c(workers.map(&:pid), options) do | ||||||||||
| in_threads(options) do |i| | ||||||||||
| in_threads(options.merge(runloop: job_factory.method(:runloop), stopper: job_factory.method(:stopper))) do |i| | ||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we reuse the options from line 408 ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, where is line 408?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have fixed and pushed the change in the same way as the other calling. Is this correct? |
||||||||||
| worker = workers[i] | ||||||||||
| worker.thread = Thread.current | ||||||||||
| queue_for_thread = Thread::Queue.new | ||||||||||
| worked = false | ||||||||||
|
|
||||||||||
| begin | ||||||||||
| loop do | ||||||||||
| break if exception | ||||||||||
| item, index = job_factory.next | ||||||||||
| item, index = job_factory.next(queue_for_thread) | ||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could the factory take care of the queue handling by using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Value of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have tried making the change which use |
||||||||||
| break unless index | ||||||||||
|
|
||||||||||
| if options[:isolation] | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| # Reproduction case based on GitHub Issue #211 | ||
| # Original code provided by @cyclotron3k in the issue | ||
|
takahiro-blab marked this conversation as resolved.
|
||
|
|
||
| require 'prime' | ||
| require './spec/cases/helper' | ||
|
|
||
| private_key = 12344567899 | ||
|
|
||
| results = [] | ||
|
|
||
| [{ in_threads: 2 }, { in_threads: 0 }].each do |options| | ||
| primes = Prime.to_enum | ||
| Parallel.each(primes, options) do |prime| | ||
| if private_key % prime == 0 | ||
| results << prime.to_s | ||
| raise Parallel::Break | ||
| end | ||
| end | ||
| end | ||
|
|
||
| print results.join(',') | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # frozen_string_literal: true | ||
| require './spec/cases/helper' | ||
|
|
||
| runner_thread = nil | ||
| all = [3, 2, 1] | ||
| my_proc = proc { | ||
| runner_thread ||= Thread.current | ||
| if Thread.current != runner_thread | ||
| raise "proc is called in different thread!" | ||
| end | ||
|
|
||
| all.pop || Parallel::Stop | ||
| } | ||
|
|
||
| class Callback | ||
| def self.call(x) | ||
| $stdout.sync = true | ||
| "ITEM-#{x}" | ||
| end | ||
| end | ||
| puts(Parallel.map(my_proc, in_threads: 2) { |(i, _id)| Callback.call i }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # frozen_string_literal: true | ||
| require './spec/cases/helper' | ||
|
|
||
| def generate_proc | ||
| count = 0 | ||
| proc { | ||
| raise StopIteration if 3 <= count | ||
| count += 1 | ||
| } | ||
| end | ||
|
|
||
| class Callback | ||
| def self.call(x) | ||
| $stdout.sync = true | ||
| "ITEM-#{x}" | ||
| end | ||
| end | ||
|
|
||
| [{ in_processes: 2 }, { in_threads: 2 }, { in_threads: 0 }].each do |options| | ||
| puts(Parallel.map(generate_proc, options) { |(i, _id)| Callback.call i }) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it not like this ?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stoppedwill be set inJobFactory#runloop.This
JobFactory#nextmay be called from some threads at the same time.So your
@stopped = (item == Stop)needs exclusive controlMutex#synchronize.Taking the value out of the
@lambdaand Setting the result to@stoppedmust be handled in the critical section or be handled in one thread, I think.Otherwise, a certain thread may clear
@stoppedflag. This could be a bug.So previous implementation of Parallel uses
@mutex.synchronize.This PR's code handles
@lambdaand checkitem == Stopin#runloopby one thread, so@mutexis given, but it's not used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thx, yeah this is a tricky section :)
can you leave a bit of inline comment for the gotchas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added comment to this code.