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

Implement opt_aref_with #297

Closed
maximecb opened this issue Nov 24, 2021 · 2 comments
Closed

Implement opt_aref_with #297

maximecb opened this issue Nov 24, 2021 · 2 comments
Labels
help wanted Extra attention is needed

Comments

@maximecb
Copy link
Contributor

It looks like the code generated by erubi makes use of opt_aref_with, and on the benchmark recently added by @noahgibbs on speed.yjit.org, this causes us to exit early, and only get 5% coverage. This instruction seems to be used to index hashes with string literals. It's not super common, but it also shouldn't be very difficult to implement, so may be nice to have.

Top exits on the erubi bechmark:

Top-20 most frequent exit ops (100.0% of exits):
    opt_aref_with:   17263742 (100.0)
      invokeblock:         26 (0.0)
      adjuststack:          0 (0.0)
      anytostring:          0 (0.0)
         branchif:          0 (0.0)
        branchnil:          0 (0.0)
@maximecb maximecb added the help wanted Extra attention is needed label Nov 24, 2021
@HParker
Copy link
Contributor

HParker commented Dec 2, 2021

I looked into this with @jhawthorn and the main complexity is that when hashes compare by identity the string needs to be rb_str_resurrect which is not impossible to do in generated code, but would be complicated. https://github.com/ruby/ruby/blob/master/insns.def#L1350-L1357

Another thing to note is Ruby won't generate opt_aref_with when frozen_string_literal is true

for example:

This generates opt_aref_with:

def foo
  h = { "hi" => "ok" }
  h["hi"]
end

This does not:

# frozen_string_literal: true
def foo
  h = { "hi" => "ok" }
  h["hi"]
end

So if this change is merged: Shopify/yjit-bench#63
it won't generate opt_aref_with anymore which maybe makes optimizing this instruction less important.

@maximecb
Copy link
Contributor Author

maximecb commented Jan 5, 2022

Roger that @HParker. We'll close this issue for now. Thanks for taking the time to investigate it 🙏

@maximecb maximecb closed this as completed Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants