-
Notifications
You must be signed in to change notification settings - Fork 112
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
Parallelize RBI parsing to speed up the check-shims
command
#1056
Conversation
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
55e7f39
to
0138278
Compare
index_rbis(index, "shim", @shim_rbi_dir, number_of_workers: @number_of_workers) | ||
index_rbis(index, "gem", @gem_rbi_dir, number_of_workers: @number_of_workers) | ||
index_rbis(index, "dsl", @dsl_rbi_dir, number_of_workers: @number_of_workers) | ||
index_rbis(index, "annotation", @annotations_rbi_dir, number_of_workers: @number_of_workers) | ||
|
||
duplicates = duplicated_nodes_from_index(index, shim_rbi_dir: @shim_rbi_dir, todo_rbi_file: @todo_rbi_file) |
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.
Just wondering for a future improvement, could the duplicates for each file be stored in a cache similar to what RuboCop does?
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.
Maybe.
A fast and elegant way would be to ask Sorbet through a custom LSP request.
@@ -250,5 +248,14 @@ def update_gem_rbis_strictnesses(errors, gem_dir) | |||
def gem_name_from_rbi_path(path) | |||
T.must(File.basename(path, ".rbi").split("@").first) | |||
end | |||
|
|||
sig { params(block: T.proc.void).returns(Numeric) } | |||
def with_timer(&block) |
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.
Could we use Benchmark
instead? I think it does exactly the same. Also, do we actually need to measure this?
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.
Could we use Benchmark instead?
Good idea!
Also, do we actually need to measure this?
I was actually thinking to extend the concept to other commands so we can easily see where Tapioca is slow or slower.
0138278
to
9cda925
Compare
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
9cda925
to
1f349b0
Compare
Motivation
Running
tapioca check-shims
can take up to 7min in Core (~13 when running on CI). The culprit is parsing the RBI files.This PR proposes to speed up the process by parallelizing RBI parsing.
Implementation
I reused
Executor
that we already use to parallelize RBI generation.Tests
No behavior change.
I tested the speed-up on Tapioca itself:
Before:
After:
So a > 50% improvement here.
I also tested on Core:
Before:
After:
So a > 50% improvement here too.