Replace RipperRubyParser with Prism for DescendantLoader #23008
Conversation
RipperRubyParser isn't really maintained anymore whereas Prism is soon becoming the replacement official Ruby parser. Prism already has a RubyParser translation layer internally, which is fully maintained. Additionally, the performance of Prism is much faster than Ripper. Performance testing DescendantLoader: Cached sti_loader? | Measurement | Time (s) ------------------ | ---------------------- | --------------------- (Ripper) No cache | Vm.descendants | 15.303 (Prism) No cache | Vm.descendants | 4.093 (73% faster) (Ripper) No cache | evm:compile_sti_loader | 20.120 (Prism) No cache | evm:compile_sti_loader | 8.216 (59% faster) (Ripper) Cached | Vm.descendants | 0.554 (Prism) Cached | Vm.descendants | 0.583 (same) (Ripper) Cached | evm:compile_sti_loader | 3.271 (Prism) Cached | evm:compile_sti_loader | 3.138 (same)
Fryguy
changed the title
There was a problem hiding this comment.
LGTM but do you think we need to verify the sti_loader.yml is unchanged between ripper and prism? I'm wondering if they're fully compatible or if there's any little differences such as strings/symbols, etc. that may require a migration or a need to recompile the loader on upgrade.
|
@jrafanie I ran a before after comparison and diff'd the sti_loader.yml files and they are identical. I also did an "upgrade" where I took a sti_loader built with the non-prism version, then built it again with prism and it was identical. I also did a test where I created a new Vm subclass model, then let it "update" the existing loader yaml and it worked as expected: diff --git a/sti_loader.yml.before b/tmp/cache/sti_loader.yml
index cfaf1e18a5..3d98f4630e 100644
--- a/sti_loader.yml.before
+++ b/tmp/cache/sti_loader.yml
@@ -26037,3 +26037,10 @@
- - WidgetImportService
- ParsedNonWidgetYamlError
- StandardError
+"/Users/jfrey/dev/manageiq/app/models/vm_flarp.rb":
+ :mtime: 2024-04-26 15:32:41.214504510 -04:00
+ :parsed:
+ - - - ''
+ - - ''
+ - VmFlarp
+ - Vm |
|
Looks good to merge when rubygems/rubygems#7607 is released and we're ready to pin the minimum version. |
|
@jrafanie rubygems/rubygems#7607 was released in 2.5.10. I've updated the PR here to ensure 2.5.10 is the minimum. Note that while the Gemfile declared >= 2.1.4 previously, I'm ok with raising the minimum because we have radjabov currently "shipping" with 2.5.5 anyway, and quinteros, interestingly, is shipping with 2.5.6. |
Fryguy
force-pushed
the
prism
branch
2 times, most recently
from
a507120
to
ed0133d
Compare
|
@jrafanie As discussed I updated the bundler version constraint. I put a 📖 for future us in the commit message and also updated the OP. This should be ready for review. |
In rubygems PR7607, this bundler bug presented itself with respect to adding the prism gem and was fixed in 2.5.10. As such, we want to avoid loading 2.5.0 to 2.5.9. Separately, the application will still work with older versions of bundler. While we could change the bundler minimum to ">= 2.5.10", and call it a day, this actually causes unnecessary overhead for developers and CI, where we are forced to install that minimum version and can't just use what comes with Ruby, which is the most convenient option. In CI, the setup-ruby action will try to use the version of bundler that comes with Ruby, and we'd like to avoid overriding that if possible, because it requires changes to ci.yaml files in every repo. Again, using what comes with Ruby is the most convenient option. So, to summarize, the goal is to support as many versions of Ruby and bundler as possible, avoid CI and developer overhead, while simultaneous not allowing specific versions of bundler, namely 2.2.10 (which has a bug that breaks our application) and 2.5.0-2.5.9, while also keeping the Gemfile line as succinct as possible. So, the changes in this commit: 1. Bump the minimum version of bundler to 2.2. This is because the application only supports Ruby 3.0+ and the version of bundler that shipped with Ruby 3.0.0 was bundler 2.2.3. 2. Bump Ruby minimum to 3.0.1 and thus further bump the bundler minimum to 2.2.15. This avoids the bundler 2.2.10 problem. It's unlikely anyone is actually using Ruby 3.0.0 since 3.0.7 is the current version of Ruby, so changing this Ruby minimum shouldn't affect many developers, and it does not affect CI at all. 3. Prevent bundler 2.5.0 through 2.5.9. The only way to do this in bundler while keeping 2.2 as the minimum is to enumerate each version. For cleanliness, this commit uses a simple map.
|
Checked commits Fryguy/manageiq@14a7757~...346e53d with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
| @@ -29,7 +29,7 @@ gem "awesome_spawn", "~>1.6", :require => false | |||
| gem "aws-sdk-s3", "~>1.0", :require => false # For FileDepotS3 | |||
| gem "bcrypt", "~> 3.1.10", :require => false | |||
| gem "bootsnap", ">= 1.8.1", :require => false # for psych 3.3.2+ / 4 unsafe_load | |||
| gem "bundler", "~> 2.1", ">= 2.1.4", "!= 2.2.10", :require => false | |||
| gem "bundler", "~> 2.2", ">= 2.2.15", *("!= 2.5.0".."!= 2.5.9"), :require => false | |||
There was a problem hiding this comment.
This is next level shorthand that is readable. 🤯 😲
There was a problem hiding this comment.
I think this might be my personal favorite thing I've ever written in Ruby
RipperRubyParser isn't really maintained anymore whereas Prism is soon becoming the replacement official Ruby parser. Prism already has a RubyParser translation layer internally, which is fully maintained. Additionally, the performance of Prism is much faster than Ripper.
Performance testing DescendantLoader:
The second commit updates bundler to deal with this new gem and some bugs found in bundler itself.
In rubygems/rubygems#7607, this bundler bug presented itself with respect to adding the prism gem and was fixed in 2.5.10. As such, we want to avoid loading 2.5.0 to 2.5.9.
Separately, the application will still work with older versions of bundler. While we could change the bundler minimum to ">= 2.5.10", and call it a day, this actually causes unnecessary overhead for developers and CI, where we are forced to install that minimum version and can't just use what comes with Ruby, which is the most convenient option. In CI, the setup-ruby action will try to use the version of bundler that comes with Ruby, and we'd like to avoid overriding that if possible, because it requires changes to ci.yaml files in every repo. Again, using what comes with Ruby is the most convenient option.
So, to summarize, the goal is to support as many versions of Ruby and bundler as possible, avoid CI and developer overhead, while simultaneous not allowing specific versions of bundler, namely 2.2.10 (which has a bug that breaks our application) and 2.5.0-2.5.9, while also keeping the Gemfile line as succinct as possible.
So, the changes in this commit:
@jrafanie Please review.
Will likely depend on rubygems/rubygems#7585, so we may not be able to merge until we change the bundler minimum.