Skip to content
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

Refactor & Optimize EmptyLineAfterSig #185

Merged
merged 4 commits into from
Oct 16, 2023
Merged

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Oct 13, 2023

The previous implementation would traverse the tokens in the file each time it would analyze a sig until it found the corresponding method definition. This would take additional time the further we got into a file, as we would have to re-traverse all tokens so far.

This new implementation simply looks for the next node after the sig, confirms it is the method definition, and calculates offsets for the lines in between.

This also migrates from this class from the deprecated Cop parent class to Base.


When run across some of the largest files in the Shopify monolith, we see a > 2,000x improvement.1

EmptyLineAfterSig#on_signature
Before After Change
Time Spent Percentage of Total Samples Time Spent Percentage of Total Samples Speedup Factor Percentage Improvement
7.10s 13% 1.97ms <0.01% 3,604x -99.97%
6.86s 13% 2.94ms <0.01% 2,333x -99.96%

Before Merging

Footnotes

  1. No fancy benchmarking as the difference is large enough. Just ran rubocop --profile a couple times toggling between branches and grabbing results from Speedscope.

@sambostock sambostock force-pushed the optimize-empty-line-after-sig branch 2 times, most recently from dab8703 to 535137f Compare October 13, 2023 15:36
@sambostock sambostock marked this pull request as ready for review October 13, 2023 15:37
@sambostock sambostock requested a review from a team as a code owner October 13, 2023 15:37
MSG = "Extra empty line or comment detected"

# @!method signable_method_definition?(node)
def_node_matcher :signable_method_definition?, <<~PATTERN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also enable this cop in the RBI config: https://github.com/Shopify/rubocop-sorbet/blob/main/config/rbi.yml#L195.

Which would mean allowing multiple signatures on the same def, so the next sibling could also be a sig.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed. Instead of next_sibling, we should still be looking for the next method/attr_ definition in a more performant way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next_sibling approach already handled the case where there was an empty line between the last signature and method definition in a multiple signature case, but it didn't handle the case where there was an empty line between multiple signatures.

However, that was an easy change: the SignatureHelp module (replacing the SignatureCop class in #183) includes a signature? node pattern, which we can use to update the node pattern we use to check if the next_sibling is relevant to also consider signatures relevant.

In the interest of keeping this PR focused on the refactor and optimization, I've made those two changes in #188 instead:

  • Teaches EmptyLineAfterSig to detect empty lines between multiple sigs
  • Enables EmptyLineAfterSig in rbi.yml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good to me!

@sambostock sambostock force-pushed the optimize-empty-line-after-sig branch 2 times, most recently from a54aa3f to 535137f Compare October 15, 2023 21:21
@sambostock sambostock force-pushed the signature-help branch 2 times, most recently from 2643055 to f1502f0 Compare October 16, 2023 01:31
Base automatically changed from signature-help to main October 16, 2023 01:45
These are unlikely, but we should handle them without errors.
The previous implementation would traverse the tokens in the file each
time it would analyze a `sig` until it found the corresponding method
definition. This would take additional time the further we got into a
file, as we would have to re-traverse all tokens so far.

This new implementation simply looks for the next node after the `sig`,
confirms it is the method definition, and calculates offsets for the
lines in between.
@sambostock sambostock merged commit 5499b1c into main Oct 16, 2023
12 checks passed
@sambostock sambostock deleted the optimize-empty-line-after-sig branch October 16, 2023 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants