Add concurrency #33
Add concurrency #33
Conversation
|
The CircleCI failure here looks unrelated. Passes locally. |
| @@ -14,6 +14,10 @@ def languages | |||
| config.fetch("languages", {}) | |||
| end | |||
|
|
|||
| def concurrency | |||
There was a problem hiding this comment.
Is this something we want users to be able to configure?
There was a problem hiding this comment.
Yes definitely, though we should probably consider having some max in place
There was a problem hiding this comment.
Yeah, that was my concern. Don't want someone going overboard with concurrency.
There was a problem hiding this comment.
I'd cap it at 2 for now. Later, we'll probably make this externally configurable by a build runner so that we can set the max to other values.
|
Instead of pulling in flay directly, what do you think about forking flay and making changes in that repo? |
BlakeWilliams
force-pushed
the
bh/concurrency
branch
2 times, most recently
from
3ba2a1d to
d96602f
Compare
|
Alright, added some changes:
|
|
cc @codeclimate/review |
| Thread.new do | ||
| begin | ||
| yield queue.pop(true) | ||
| rescue ThreadError |
There was a problem hiding this comment.
Since we're rescuing here, how would you feel about logging that this happened?
There was a problem hiding this comment.
Yeah, that's a good idea. I'll print something to stderr.
There was a problem hiding this comment.
This rescue is actually just for when queue.pop runs out of files to pop, so I don't think it should log to stderr.
|
Few comments otherwise this LGTM. |
This PR swaps out flay with our own fork of flay that uses concurrency-ruby classes instead of stdlib `Hash`. This also introduces a `FileThreadPool` class for running blocks on an array of files concurrently.
BlakeWilliams
force-pushed
the
bh/concurrency
branch
from
8ea2836 to
ce27d9e
Compare
Adds thread-based concurrency to the parsing and
process_sexpphase (but not the "report" phase at the end). Concurrency is implemented using a vanilla RubyQueueand threads. The concurrency is configurable in the engine config (which I think we will want to add to the spec, and defaults to 2).This uses the
Concurrent::Array,Concurrent::HashandConcurrent::Mapclasses from theconcurrent-rubygem for thread safe data structures in Flay (which is why I unpackedflay.rbfrom the Gem dependency to patch it.We should consider submitting a concurrency patch upstream after we test this more. It will require some re-work but it's manageable.
Note: The last step is switching the Ruby interpreter from MRI to JRuby in the Dockerfile (but this should produce wins even on MRI for non-Ruby languages due to the concurrent parsing in the e.g. Node.js processes we boot).
/c @jpignata