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
Disable iseq cache in Ruby 2.5 #257
Conversation
Won’t this turn on iseq cache for 2.5 only? As far as I understand the logic currently implemented. |
@kaspth good catch. It should be reversed. |
There are many bugs in Ruby 2.5 related with iseq cache making it hard to use tracepoints with it. This means that debuggers and any gem that uses tracepoint don't play well with bootsnap when used with Ruby 2.5. Until those issues are fixed in Ruby 2.5 we are going to disabled iseq cache in the `bootsnap/setup` file.
6f6c20e
to
7631334
Compare
Fixed |
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 now! 💪
@@ -24,12 +24,15 @@ | |||
cache_dir = File.join(app_root, 'tmp', 'cache') | |||
end | |||
|
|||
ruby_version = Gem::Version.new(RUBY_VERSION) | |||
iseq_cache_enabled = ruby_version < Gem::Version.new('2.5.0') || ruby_version >= Gem::Version.new('2.6.0') |
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 for my own curiosity, why are these matchers more correct versus something like RUBY_VERSION !~ /\A2\.5/
?
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.
In this case they are not, I was just following the same pattern other parts of the codebase was using.
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.
Ah, I see. All good 👍
Really the issue is when we're using 2.5.x and also Rails 6+, right? Is there some simple way you can think of to cheaply check that without throwing away performance for apps that haven't updated either? |
No. Even without Rails 6, if you have iseq cache enable, debugging doesn't work well due to a bug where trace points are not fired when iseq cache is enabled. See #168 and related issues. |
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.
Ok, seems fine then. Also FWIW, don't feel like you need to wait for my review if I'm away and you're confident in the change.
I just saw that they marked @XrXr's to backport to 2.5, that should fix our issue so I'll remove this code when Ruby 2.5.5 is released. |
Ref: #257 The fix was backported in 2.5.4, so we can restrict the range to 2.5.0...2.5.3.
There are many bugs in Ruby 2.5 related with iseq cache making it hard to use tracepoints with it. This means that debuggers and any gem that uses tracepoint don't play well with bootsnap when used with Ruby 2.5.
Until those issues are fixed in Ruby 2.5 we are going to disabled iseq cache in the
bootsnap/setup
file.Fixes rails/rails#35475.