-
Notifications
You must be signed in to change notification settings - Fork 256
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 all 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,30 @@ 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 | ||||||||||
@worker_queues = Thread::Queue.new if @lambda | ||||||||||
@mutex = mutex | ||||||||||
@index = -1 | ||||||||||
@stopped = false | ||||||||||
end | ||||||||||
|
||||||||||
def next | ||||||||||
if producer? | ||||||||||
queue_for_thread = Thread.current.thread_variable_get(:parallel_queue) | ||||||||||
if @worker_queues && queue_for_thread | ||||||||||
return if @stopped | ||||||||||
# This #next method may be called from some threads at the same time. | ||||||||||
# The main (Parallel's singleton method caller) thread calls @lambda and checks `item == Stop`, | ||||||||||
# so it's not necessary to check for Stop here. | ||||||||||
item = worker_queues_enq(queue_for_thread) | ||||||||||
return if item == Stop | ||||||||||
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 +133,26 @@ def next | |||||||||
[item, index] | ||||||||||
end | ||||||||||
|
||||||||||
def consume_worker_queues | ||||||||||
return unless @worker_queues | ||||||||||
|
||||||||||
loop do | ||||||||||
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 = @worker_queues.pop | ||||||||||
return if queue == Stop | ||||||||||
item = call_lambda | ||||||||||
queue.push(item) | ||||||||||
break if item == Stop | ||||||||||
end | ||||||||||
@stopped = true | ||||||||||
# clear out all work queues by adding a "stop" to them which will stop the thread working on them | ||||||||||
begin | ||||||||||
takahiro-blab marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
while queue = @worker_queues.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 +172,36 @@ def unpack(data) | |||||||||
producer? ? data : [@source[data], data] | ||||||||||
end | ||||||||||
|
||||||||||
def stop | ||||||||||
@worker_queues&.push(Stop) | ||||||||||
end | ||||||||||
|
||||||||||
private | ||||||||||
|
||||||||||
def call_lambda | ||||||||||
@lambda.call | ||||||||||
rescue StopIteration | ||||||||||
Stop | ||||||||||
end | ||||||||||
|
||||||||||
def worker_queues_enq(queue_for_thread) | ||||||||||
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 also work if we did:
so the caller has less state to take care of 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. ... also maybe method name 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. Your code also works. However, well, your code makes as many queues as jobs. Those queues remain in memory until they are collected by the GC. I do not consider creating a new queue for every jobs a good implementation. In a frank implementation, I would think the queues would be reused, but why make a queue every time? 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. makes sense to reuse the queues, was mostly trying to understand if that's how it is supposed to work :) |
||||||||||
@worker_queues.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 | ||||||||||
|
||||||||||
# returns the `next` method if the source in an enumerator and cannot be accessed by anything more efficient like [] | ||||||||||
def enum_wrapper(source) | ||||||||||
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 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.
Show resolved
Hide resolved
|
||||||||||
# Convert what is inaccessible by the index | ||||||||||
!source.respond_to?(:[]) && source.respond_to?(:next) && source.method(:next) | ||||||||||
end | ||||||||||
end | ||||||||||
|
||||||||||
class UserInterruptHandler | ||||||||||
|
@@ -211,13 +262,28 @@ 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) | ||||||||||
|
||||||||||
counter = count # worker thread remaining counter | ||||||||||
mutex = options[:consume_worker_queues] ? Mutex.new : nil | ||||||||||
consume_worker_queues_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 | ||||||||||
mutex&.synchronize do | ||||||||||
if counter <= 1 | ||||||||||
# last thread needs to stop the worker queue processing | ||||||||||
consume_worker_queues_stopper.call | ||||||||||
end | ||||||||||
counter -= 1 | ||||||||||
end | ||||||||||
end | ||||||||||
end | ||||||||||
options[:consume_worker_queues]&.call # Invoke lambda in caller thread, and provide jobs to thread queue. | ||||||||||
threads.map(&:value) | ||||||||||
end | ||||||||||
ensure | ||||||||||
|
@@ -431,7 +497,9 @@ 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| | ||||||||||
thread_options = options.merge(consume_worker_queues: job_factory.method(:consume_worker_queues), stopper: job_factory.method(:stop)) | ||||||||||
in_threads(thread_options) do |worker_num| | ||||||||||
Thread.current.thread_variable_set(:parallel_queue, 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) | ||||||||||
|
@@ -523,9 +591,11 @@ 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| | ||||||||||
thread_options = options.merge(consume_worker_queues: job_factory.method(:consume_worker_queues), stopper: job_factory.method(:stop)) | ||||||||||
in_threads(thread_options) do |i| | ||||||||||
worker = workers[i] | ||||||||||
worker.thread = Thread.current | ||||||||||
Thread.current.thread_variable_set(:parallel_queue, Thread::Queue.new) | ||||||||||
worked = false | ||||||||||
|
||||||||||
begin | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# 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.
Show resolved
Hide resolved
|
||
# using a enum that is infinite, so this will hang forever when trying to convert to an array | ||
|
||
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.
comment says that we don't need to check for stop but then we check for stop ?
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.
This "check for stop" means assigning
item == Stop
to@stopped
. This comment means "We must not assign check result to@stopped
." The comment does not refer to the returning....