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

YJIT: Optimize defined?(yield) #9599

Merged
merged 3 commits into from
Jan 19, 2024
Merged

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Jan 18, 2024

This PR optimizes defined instruction for the DEFINED_YIELD case, which is used by defined?(yield).

It has been raised as an idea to use defined?(yield) instead of block_given? when we rewrite a C method in Ruby so that it will not be impacted by method redefinition.

Matz didn't ask for tolerating such redefinition, so we don't need to do it for compatibility. However, it could be still useful for performance when we rewrite C methods in Ruby since defined?(yield) is not a method call unlike block_given?, so it's faster on the interpreter.

Even for YJIT, defined instruction is more preferable because you don't need to split blocks for defer_compilation (smaller memory overhead) or add a guard for the receiver (faster).

Since we're interested in removing C -> Ruby calls, we would naturally rewrite methods that check block_given?. To make it easier for us to merge such changes, being able to use this optimization for the interpreter performance seems useful.

Example code

block_given?

  # Insn: 0001 opt_send_without_block (stack_size: 1)
  # call to Object#block_given?
  # guard known object with singleton class
  0x559640a680bf: movabs rax, 0x7f9f7f6ace00
  0x559640a680c9: cmp rsi, rax
  0x559640a680cc: jne 0x559640a6a0aa
  # block_given?
  0x559640a680d2: mov rax, qword ptr [r13 + 0x20]
  0x559640a680d6: mov rax, qword ptr [rax - 8]
  0x559640a680da: test rax, rax
  0x559640a680dd: mov eax, 0x14
  0x559640a680e2: mov ecx, 0
  0x559640a680e7: cmove rax, rcx
  0x559640a680eb: mov rsi, rax

defined?(yield)

  # Insn: 0001 defined (stack_size: 1)
  # block_given?
  0x559640a6805d: mov rax, qword ptr [r13 + 0x20]
  0x559640a68061: mov rax, qword ptr [rax - 8]
  0x559640a68065: test rax, rax
  0x559640a68068: movabs rax, 0x7f9f7f671940
  0x559640a68072: mov rax, rax
  0x559640a68075: mov ecx, 4
  0x559640a6807a: cmove rax, rcx
  0x559640a6807e: mov rsi, rax

@matzbot matzbot requested a review from a team January 18, 2024 23:03
Copy link
Member

@XrXr XrXr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat method-call free way to do block_given? without taking a block param!

@jeremyevans
Copy link
Contributor

Sequel and Roda have both been using defined?(yield) instead of block_given? for a couple years now, so I'm definitely in favor of further optimizing it.

@luke-gru
Copy link
Contributor

Is it not possible to replace block_given? with defined(yield) in the peephole optimizer? I guess a user could have redefined the method, but why would they want to?

@XrXr
Copy link
Member

XrXr commented Jan 19, 2024

Is it not possible to replace block_given? with defined(yield) in the peephole optimizer? I guess a user could have redefined the method, but why would they want to?

We can't make sneaky language semantics changes like that, it's too surprising for users and too hard to discover. Also, there is already many redefinitions: https://github.com/search?q=%22def+block_given%3F%22&type=code

@maximecb
Copy link
Contributor

  0x559640a68072: mov rax, rax

🥲

Copy link
Contributor

@maximecb maximecb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Alan. This is neat. Good thinking :)

@maximecb maximecb merged commit 3c92901 into ruby:master Jan 19, 2024
96 checks passed
@k0kubun k0kubun deleted the yjit-defined-yield branch January 19, 2024 16:56
@byroot
Copy link
Member

byroot commented Jan 22, 2024

For people stumbling on this and thinking to write a rubocop-performance Cop for it, I beg you not to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants