YJIT: Optimize attr_writer #9986
Conversation
k0kubun
force-pushed
the
yjit-attrset
branch
13 times, most recently
from
e427b28
to
7e9af54
Compare
k0kubun
force-pushed
the
yjit-attrset
branch
from
7e9af54
to
f27198f
Compare
| if let StackOpnd(index) = recv_opnd { // attr_writer | ||
| let recv = asm.stack_opnd(index as i32); | ||
| asm_comment!(asm, "call rb_vm_set_ivar_id()"); | ||
| asm.ccall( | ||
| rb_vm_set_ivar_id as *const u8, | ||
| vec![ | ||
| recv, | ||
| Opnd::UImm(ivar_name), | ||
| val_opnd, | ||
| ], | ||
| ); | ||
| } else { // setinstancevariable | ||
| asm_comment!(asm, "call rb_vm_setinstancevariable()"); | ||
| asm.ccall( | ||
| rb_vm_setinstancevariable as *const u8, | ||
| vec![ | ||
| Opnd::const_ptr(jit.iseq as *const u8), | ||
| Opnd::mem(64, CFP, RUBY_OFFSET_CFP_SELF), | ||
| ivar_name.into(), | ||
| val_opnd, | ||
| Opnd::const_ptr(ic.unwrap() as *const u8), | ||
| ], | ||
| ); | ||
| } |
There was a problem hiding this comment.
It seems like a good idea to reuse code and it's nice to see a 2% speedup! 👍
However, this PR introduces multiple if statements to use different operands and change the behavior, which seems a bit weird.
Just to make sure I understand, attr_writers do not have ICs? That's why you have to call a different function here?
There was a problem hiding this comment.
You're right. attr_writers don't have ICs we need here, so attr_writers can't use rb_vm_setinstancevariable. An attr_writer is a method call, so it has an cd->cc (method call cache), but it doesn't have an instance-variable cache ic (IVC), which is used by setinstancevariable.
Similarly, setinstancevariable can't use rb_vm_set_ivar_id, which supports only T_OBJECT. attr_writer fallbacks to a dynamic dispatch when the receiver is not T_OBJECT, but setinstancevariable wants to use this general case for non-T_OBJECT cases.
We could use rb_ivar_set for both cases, which supports non-T_OBJECT receivers as well, but I saw a slowdown when I tried that, so I kept this logic. But let me benchmark it again 🤔
There was a problem hiding this comment.
We could use rb_ivar_set for both cases, which supports non-T_OBJECT receivers as well, but I saw a slowdown when I tried that, so I kept this logic. But let me benchmark it again 🤔
It does seem to give a slowdown (these benchmarks consistently give a 0.99x change). It seems to hurt the performance if you handle both cases using rb_ivar_set. Another thing we could try is to replace rb_vm_setinstancevariable with rb_ivar_set, but it still leaves this if branch, so it doesn't simplify code.
before: ruby 3.4.0dev (2024-02-22T01:26:29Z yjit-attrset f27198fac3) +YJIT [x86_64-linux]
after: ruby 3.4.0dev (2024-02-22T17:12:46Z yjit-attrset 2b73548263) +YJIT [x86_64-linux]
---------- ----------- ---------- ---------- ---------- ------------- ------------
bench before (ms) stddev (%) after (ms) stddev (%) after 1st itr before/after
hexapdf 1862.5 3.0 1874.3 1.9 1.02 0.99
railsbench 2423.5 0.9 2436.3 0.8 0.98 0.99
---------- ----------- ---------- ---------- ---------- ------------- ------------
There was a problem hiding this comment.
Another thing we could try is to replace rb_vm_setinstancevariable with rb_ivar_set, but it still leaves this if branch, so it doesn't simplify code.
I also tried this, but it also gave a slowdown (0.99x) on railsbench. So it seems more efficient to keep using rb_vm_setinstancevariable for setinstancevariable and rb_vm_set_ivar_id for attr_writer.
There was a problem hiding this comment.
Oh yeah, I wasn't suggesting that we use the less efficient function for both. The code is a bit harder to follow though with the special casing, but I understand why it is there.
This PR optimizes
attr_writerby reusingsetinstancevariable's shape-based implementationgen_set_ivar.This seems to speed up railsbench by 2%.