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

use correct svar #7225

Merged
merged 2 commits into from Feb 2, 2023
Merged

use correct svar #7225

merged 2 commits into from Feb 2, 2023

Conversation

ko1
Copy link
Contributor

@ko1 ko1 commented Feb 1, 2023

Without this patch, svar location is used "nearest Ruby frame". It is almost correct but it doesn't correct when the each method is written in Ruby.

class C
  include Enumerable
  def each
    %w(bar baz).each{|e| yield e}
  end
end

C.new.grep(/(b.)/){|e| p [$1, e]}

This patch fix this issue by traversing ifunc's cfp.

Note that if cfp doesn't specify this Thread's cfp stack, reserved svar location (ec->root_svar) is used.

Without this patch, svar location is used "nearest Ruby frame".
It is almost correct but it doesn't correct when the `each` method
is written in Ruby.

```ruby
class C
  include Enumerable
  def each
    %w(bar baz).each{|e| yield e}
  end
end

C.new.grep(/(b.)/){|e| p [$1, e]}
```

This patch fix this issue by traversing ifunc's cfp.

Note that if cfp doesn't specify this Thread's cfp stack, reserved
svar location (`ec->root_svar`) is used.
@k0kubun k0kubun marked this pull request as ready for review February 2, 2023 00:02
@matzbot matzbot requested a review from a team February 2, 2023 00:04
@k0kubun k0kubun removed the request for review from a team February 2, 2023 00:04
@k0kubun
Copy link
Member

k0kubun commented Feb 2, 2023

requested a review from https://github.com/orgs/ruby/teams/yjit

Wrong ping. We did #7004 to skip this kind of PR, but it's disabled for now 74e52c2 because it had a bug.


The patch makes sense to me, thanks! This doesn't have a significant impact on performance either:

before: ruby 3.3.0dev (2023-02-01T21:05:22Z master 2675f2c864) [x86_64-linux]
last_commit=Remove whitespace
after: ruby 3.3.0dev (2023-02-01T23:59:25Z use_correct_ivar 7a81a4d59a) [x86_64-linux]

----------  -----------  ----------  ----------  ----------  ------------  -------------
bench       before (ms)  stddev (%)  after (ms)  stddev (%)  before/after  after 1st itr
railsbench  1551.1       0.6         1555.0      0.5         1.00          1.00
----------  -----------  ----------  ----------  ----------  ------------  -------------

I'll need this for #6687 and it doesn't hurt to correct this implementation, so let me merge this.

@k0kubun k0kubun merged commit 0a82bfe into ruby:master Feb 2, 2023
k0kubun added a commit to Shopify/ruby that referenced this pull request Feb 2, 2023
k0kubun added a commit to Shopify/ruby that referenced this pull request Feb 2, 2023
k0kubun added a commit to Shopify/ruby that referenced this pull request Feb 2, 2023
k0kubun added a commit that referenced this pull request Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants