[WiP] Parallel Rubocop, fixes #117 #175
Conversation
|
Coverage decreased (-0%) when pulling ce6599dc0cd0e9327c7bb0d9a3309828cd10985c on jurriaan:parallel into 9f34175 on bbatsov:master. |
|
It's not bug free, still some bugs left. it is not displaying errors for example. |
| inspect_file(file, source, config, report) | ||
| end | ||
| report | ||
| end.each { |report| report.display unless report.empty? } |
There was a problem hiding this comment.
I'd extract this into a separate method display_report to make code a bit more clear.
There was a problem hiding this comment.
Why not integrate it with the display_summary method?
There was a problem hiding this comment.
It's easier to test stuff when they are decoupled, that's why I suggested a separate method.
|
4 times speedup is most impressive! And we'll get another big boost when @whitequark releases Parser 2.0, since it contains a crucial performance optimization. I'll have a look at the code now. :-) |
|
The code looks good to me. |
|
Coverage remained the same when pulling 4f9515ddebbba99f5801c772a178798258b58261 on jurriaan:parallel into 9f34175 on bbatsov:master. |
|
Coverage decreased (-5.02%) when pulling 4f9515ddebbba99f5801c772a178798258b58261 on jurriaan:parallel into 9f34175 on bbatsov:master. |
| # no more checking in the file. | ||
| report << syntax_cop | ||
| @total_offences += syntax_cop.offences.count | ||
| # In case of a syntax error we just report that error and do |
There was a problem hiding this comment.
Oops. Incorrect indentation, right? Should be two spaces.
There was a problem hiding this comment.
Fixing it in next commit. :)
|
The |
|
@jurriaan Sounds reasonable to me. Breaking big methods into smaller ones is rarely a bad idea :-) |
|
Coverage decreased (-0%) when pulling 021793848f1d43c120be256cd6ff1ccbdbd28e05 on jurriaan:parallel into 9f34175 on bbatsov:master. |
|
@bbatsov I'm having some trouble trying to rebase this on the latest master. Errors are now stored in a instance variable of My previous solution was adding the errors to the reports. Should I change the current functionality so it does add them to the reports again? What do you think? |
|
I think you should do some tests with threads on MRI - IO ops should be parallelized even there. You'd also do well to compare process vs thread performance on MRI/Rubinius and JRuby now that we support them all. If we decided to use processes storing the errors in the reports is a viable idea. |
|
It's much faster if you use processes on MRI. |
|
@bbatsov What do you think? I don't want to wait too long with rebasing. |
|
@jurriaan I'm worried that spawning processes on JRuby(I guess this will spin a few JVMs) might be very slow. Do a test with it to make sure it's performing ok and we can proceed. Alternatively we might simply check the RUBY_ENGINE and select threads or processes based on it. |
|
It's not possible to use threads at the moment because of the |
|
@jurriaan That's a reasonable idea. Go ahead and extract the parsing logic. |
|
BTW, I think that parallel gem uses threads on JRuby. |
|
Correct, fork() doesnt work by default on jruby On 19 mei 2013, at 21:12, Peter Zotov [email protected] wrote:
|
|
@jurriaan That means you'll definitely need to extract parsing away. |
|
@bbatsov I'll create a separate pull request for that when I have time :) |
See #117
Changed a lot of things to get it to work correctly.
Including some hacks in the specs.
Feedback would be appreciated 😄