Benchmark iterator, avoid redundant queue, remove managers. #3119
Conversation
|
Thanks, feedback addressed! |
| # Half of 100 files * 4 sentences / file | ||
| i = 0 | ||
| for instance in reader.read(self.identical_files_glob): | ||
| if i == 200: |
There was a problem hiding this comment.
I would probably just add a comment here, along the lines of "i == 200 means this is the 201st instance, which is too many, so break out and fail the test"
There was a problem hiding this comment.
Good call, done
…lp into speedups-single-queue
|
Thanks for the review! (I'm dragging my heels on merging due to some test discrepancies between TC and my local machine. I'll ping for another pass if the fix is anything major.) |
| @@ -33,13 +36,51 @@ def instances() -> Iterator[Instance]: | |||
|
|
|||
| output_queue.put(index) | |||
|
|
|||
| # We need to ensure we've gotten all the tensors out of this queue before | |||
There was a problem hiding this comment.
@joelgrus, if you're interested, this comment describes the fix for the issue I alluded to earlier. Given all these edge cases I see why you preferred using a Manager in the first version of this code!
| https://eli.thegreenplace.net/2009/06/12/safely-using-destructors-in-python/. | ||
| """ | ||
| for process in self.processes: | ||
| process.terminate() |
There was a problem hiding this comment.
@joelgrus, last bug should be fixed now. It was a nasty race in the tests due to the stray child processes. I ended up using __del__ here for that. I know that using with is preferred in Python, but I'm not sure we have that as an option given that we're attempting to fit the Iterable interface. We aren't holding any circular refs here, so this should be safe, IIUC. Certainly it fixes the problem empirically. Any concerns?
There was a problem hiding this comment.
if it works, I have no concerns 😇
(I am somewhat far from this code at this point, and I'm not really a multiprocessing expert)
|
Thanks for the review! |
…3119) - Adds a script to benchmark iterators. - Average speed - Introspects queues - Removes a bottleneck when `MultiprocessDatasetReader` and `MultiprocessIterator` are used in conjunction. - Specifically, removes a redundant queue that was populated by a single process. - Removes managers which have significant overhead. - Results on training_config/bidirectional_language_model.jsonnet: - original code, no multiprocessing: 0.047 s/b over 10000 batches - original code, workers = 1: 0.073 s/b over 10000 batches - original code, workers = 10: 0.078 s/b over 10000 batches - this PR (-queue), workers = 1: 0.073 s/b over 10000 batches - this PR (-queue), workers = 10: 0.046 s/b over 10000 batches - this PR (-queue, - manager), workers = 1: 0.063 s/b over 10000 batches - this PR (-queue, - manager), workers = 10: 0.020 s/b over 10000 batches - Notably, previously we did not see any benefit from scaling to multiple workers. Now we do, albeit worse than linearly. More work required there. - Related issues: allenai#2962, allenai#1890
MultiprocessDatasetReaderandMultiprocessIteratorare used in conjunction.MultiProcessDatasetReader#1890