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

Jump to bottom

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.

@ko1
use correct svar
59d2031 
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 •
edited

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
89 of 93 checks passed
k0kubun added a commit to Shopify/ruby that referenced this pull request Feb 2, 2023
@k0kubun
Add a test for svar ruby#7225
0d7be84
k0kubun added a commit to Shopify/ruby that referenced this pull request Feb 2, 2023
@k0kubun
Add a test for svar ruby#7225
834cc0f
k0kubun added a commit to Shopify/ruby that referenced this pull request Feb 2, 2023
@k0kubun
Add a test for svar ruby#7225
97426c1
k0kubun added a commit that referenced this pull request Feb 2, 2023
@k0kubun
Add a test for svar #7225 (#7228)
1c5bd01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@ko1 @k0kubun