-
-
Notifications
You must be signed in to change notification settings - Fork 56
Enable reusable Prism parse result #359
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
Conversation
Follow-up to Shopify/ruby-lsp#1849. This is an extension of `Prism::Translation::Parser` to implement Shopify/ruby-lsp#1849. It is based on the comments in Shopify/ruby-lsp#1849 (review), but also adds a default argument for delegation to `Parser::Base` super class. Using this API, rubocop/rubocop-ast#359 has been implemented in RuboCop AST. As detailed in rubocop/rubocop-ast#359, this change is expected to improve performance by 1.3x for some source code. Achieving a 1.3x speedup with such this simple modification is a significant improvement for Ruby LSP and its users.
Follow-up to Shopify/ruby-lsp#1849 This feature utilizes ruby/prism#3478 and rubocop/rubocop-ast#359 to implement Shopify/ruby-lsp#1849. As described in rubocop/rubocop-ast#359, it speeds up the processing around `Prism::Translation::Parser` via `ProcessedSource` by 1.3x when using the Ruby LSP add-on.
Follow-up to Shopify/ruby-lsp#1849 This feature utilizes ruby/prism#3478 and rubocop/rubocop-ast#359 to implement Shopify/ruby-lsp#1849. As described in rubocop/rubocop-ast#359, it speeds up the processing around `Prism::Translation::Parser` via `ProcessedSource` by 1.3x when using the Ruby LSP add-on.
890ca0d
to
29938d0
Compare
Follow-up to Shopify/ruby-lsp#1849. This is an extension of `Prism::Translation::Parser` to implement Shopify/ruby-lsp#1849. It is based on the comments in Shopify/ruby-lsp#1849 (review), but also adds a default argument for delegation to `Parser::Base` super class. Using this API, rubocop/rubocop-ast#359 has been implemented in RuboCop AST. As detailed in rubocop/rubocop-ast#359, this change is expected to improve performance by 1.3x for some source code. Achieving a 1.3x speedup with such this simple modification is a significant improvement for Ruby LSP and its users.
I'm OK with the proposal, but I'd suggest to document this optimization both here and eventually in RuboCop.
One thing that's not very clear to me - when/how does this pre-parsing happen in Prism? |
Ah, after reviewing the RubyLSP thread it seems they are already using |
lib/rubocop/ast/processed_source.rb
Outdated
# | ||
# This class implements the `parse_lex` method to return a preparsed `Prism::ParseLexResult` | ||
# rather than parsing the source code. It is useful in use cases like Ruby LSP, | ||
# where parsed results are reused across multiple language servers, improving performance. |
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.
Are you sure that wording is correct here? (between multiple language servers) I'd assume the benefit here is that the same result is reused for different operations within the lifetime of a specific instance of the language server.
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.
I've updated the text to focus on an objective expectation instead of providing an example for a use case that could potentially be misleading.
Follow-up to Shopify/ruby-lsp#1849. This feature utilizes ruby/prism#3478 to implement Shopify/ruby-lsp#1849. By using ruby/prism#3478, this feature enables performance improvements by reusing Prism's parsed results instead of parsing the source code. Below is a sample code snippet, which is expected to improve performance by 1.3x in this case. ```ruby #!/usr/local/bin/ruby require 'benchmark/ips' require 'prism' require 'rubocop-ast' @source = File.read(__FILE__) @parse_lex_result = Prism.parse_lex(@source) def build_processed_source(parse_lex_result) RuboCop::AST::ProcessedSource.new( @source, 3.4, __FILE__, parser_engine: 'parser_prism', prism_result: parse_lex_result ) end Benchmark.ips do |x| x.report('source') { build_processed_source(nil) } x.report('Prism::ParseLexResult') { build_processed_source(@parse_lex_result) } x.compare! end ``` ```console $ bundle exec ruby reusable_ast.rb ruby 3.4.2 (2025-02-15 revision d2930f8e7a) +PRISM [x86_64-darwin23] Warming up -------------------------------------- source 116.000 i/100ms Prism::ParseLexResult 151.000 i/100ms Calculating ------------------------------------- source 1.144k (± 8.4%) i/s - 5.684k in 5.018270s Prism::ParseLexResult 1.460k (± 5.2%) i/s - 7.399k in 5.082102s Comparison: Prism::ParseLexResult: 1460.3 i/s source: 1144.4 i/s - 1.28x slower ``` Achieving 1.3x speedup with such this simple modification is a significant improvement for Ruby LSP and its users. ## Compatibility By using `parser_class.instance_method(:initialize).parameters.assoc(:key)`, the implementation checks whether `Prism#initialize` supports the `:parser` keyword. If it does not, the system falls back to the conventional parsing method. This ensures backward compatibility with previous versions of Prism. ## Development Notes Since this feature is specifically designed for Prism, the keyword name `prism_result` is meant for Prism. While it may be possible to achieve similar functionality with the Parser gem, there are currently no concrete use cases. Therefore, for clarity, we have chosen `prism_result` as the keyword. Additionally, Prism has two types of results: `Prism::ParseResult` and `Prism::ParseLexResult`, but the `prism_result` keyword argument is meant to receive `Prism::ParseLexResult`. Since `prism_parse_lex_result` would be too long, the name `prism_result` was chosen to align with the superclass `Prism::Result`.
29938d0
to
21580cb
Compare
I've added it to the index.adoc.
Yes, that understanding is correct. For Ruby LSP, re-parsing the same source code in RuboCop, when it has already been parsed externally, is redundant processing. This is an optimization for Ruby LSP and does not directly speed up RuboCop itself. |
Follow-up to Shopify/ruby-lsp#1849 This feature utilizes ruby/prism#3478 and rubocop/rubocop-ast#359 to implement Shopify/ruby-lsp#1849. As described in rubocop/rubocop-ast#359, it speeds up the processing around `Prism::Translation::Parser` via `ProcessedSource` by 1.3x when using the Ruby LSP add-on.
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.
Looks good to me. 🚀
Follow-up to Shopify/ruby-lsp#1849. This is an extension of `Prism::Translation::Parser` to implement Shopify/ruby-lsp#1849. It is based on the comments in Shopify/ruby-lsp#1849 (review), but also adds a default argument for delegation to `Parser::Base` super class. Using this API, rubocop/rubocop-ast#359 has been implemented in RuboCop AST. As detailed in rubocop/rubocop-ast#359, this change is expected to improve performance by 1.3x for some source code. Achieving a 1.3x speedup with such this simple modification is a significant improvement for Ruby LSP and its users. ruby/prism@925725291c
@bbatsov ruby/prism#3478 has been merged. Since it is compatible with both the old and new versions of Prism, once this PR is merged and RuboCop AST is released, I will update and rebase RuboCop's rubocop/rubocop#13899. |
@vinistock I think that this feature is likely valuable for Ruby LSP, independent of rubocop/rubocop#13899. Related development is currently in progress in Shopify/ruby-lsp#3252, so if the release of this feature is needed, please feel free to let me know. |
@koic Seems to me we can go ahead and merge this without having to wait for @vinistock to respond. |
@bbatsov Yah, I'll merge this implementation as it doesn't cause any incompatibilities. Thank you! |
Thanks. @koic do you need a release for this? |
@marcandre When released, it will help with the development of rubocop/rubocop#13899 and Shopify/ruby-lsp#3252. By the way, #363 is expected to work from the next release onward :-) |
Follow-up to Shopify/ruby-lsp#1849 This feature utilizes ruby/prism#3478 and rubocop/rubocop-ast#359 to implement Shopify/ruby-lsp#1849. As described in rubocop/rubocop-ast#359, it speeds up the processing around `Prism::Translation::Parser` via `ProcessedSource` by 1.3x when using the Ruby LSP add-on.
Follow-up to Shopify/ruby-lsp#1849 This feature utilizes ruby/prism#3478 and rubocop/rubocop-ast#359 to implement Shopify/ruby-lsp#1849. As described in rubocop/rubocop-ast#359, it speeds up the processing around `Prism::Translation::Parser` via `ProcessedSource` by 1.3x when using the Ruby LSP add-on.
Follow-up to Shopify/ruby-lsp#1849 This feature utilizes ruby/prism#3478 and rubocop/rubocop-ast#359 to implement Shopify/ruby-lsp#1849. As described in rubocop/rubocop-ast#359, it speeds up the processing around `Prism::Translation::Parser` via `ProcessedSource` by 1.3x when using the Ruby LSP add-on.
Follow-up to Shopify/ruby-lsp#1849 This feature utilizes ruby/prism#3478 and rubocop/rubocop-ast#359 to implement Shopify/ruby-lsp#1849. As described in rubocop/rubocop-ast#359, it speeds up the processing around `Prism::Translation::Parser` via `ProcessedSource` by 1.3x when using the Ruby LSP add-on.
Follow-up to Shopify/ruby-lsp#1849.
This feature utilizes ruby/prism#3478 to implement Shopify/ruby-lsp#1849.
By using ruby/prism#3478, this feature enables performance improvements by reusing Prism's parsed results instead of parsing the source code. Below is a sample code snippet, which is expected to improve performance by 1.3x in this case.
Achieving 1.3x speedup with such this simple modification is a significant improvement for Ruby LSP and its users.
Compatibility
By using
parser_class.instance_method(:initialize).parameters.assoc(:key)
, the implementation checks whetherPrism#initialize
supports the:parser
keyword. If it does not, the system falls back to the conventional parsing method. This ensures backward compatibility with previous versions of Prism.Development Notes
Since this feature is specifically designed for Prism, the keyword name
prism_result
is meant for Prism. While it may be possible to achieve similar functionality with the Parser gem, there are currently no concrete use cases. Therefore, for clarity, we have chosenprism_result
as the keyword.Additionally, Prism has two types of results:
Prism::ParseResult
andPrism::ParseLexResult
, but theprism_result
keyword argument is meant to receivePrism::ParseLexResult
. Sinceprism_parse_lex_result
would be too long, the nameprism_result
was chosen to align with the superclassPrism::Result
.cc @bbatsov @marcandre @vinistock