Conditional assignment of field does two lookups

[headius]

For a simple @a ||= foo it appears we are looking for @a twice, as shown in this IR:

[] jruby $ ruby -Xir.print -e '@a ||= object_id'
2023-08-16T00:10:34.389-05:00 [main] INFO JVMVisitor : Printing JIT IR for -e:
begin SCRIPT_BODY<-e>
flags: [BINDING_HAS_ESCAPED, REQUIRES_DYNSCOPE, REQUIRES_BLOCK, FLAGS_COMPUTED]
signature(pre=0,opt=0,post=0,rest=NONE,kwargs=0,kwreq=0,kwrest=-1)

block #1 (out: 7,2): LBL_2:-1
  0:  %v_4 := copy(nil<>)
  1:          push_method_frame(frameName: -e, visibility: PRIVATE)
  2:          push_method_binding

block #2 (out: 3,4,8): LBL_3:-1
  0: %self := recv_self
  1:          line_num(lineNumber: 0, coverage: false, oneshot: false)
  2:  %v_3 := runtime_helper(self<%self>, fstr<@a>, fstr<instance-variable>, helperMethod: IS_DEFINED_INSTANCE_VAR)
  3:  %v_2 := copy(%v_3)
  4:          b_nil(ipc<LBL_1:10>, %v_3, jumpTarget: LBL_1:10, value: %v_3)

block #3 (out: 4,8): LBL_4:-1
  0:  %v_4 := get_field(self<%self>, name: @a)
  1:  %v_2 := copy(%v_4)

block #4 (out: 5,6,8): LBL_1:10
  0:          b_true(ipc<LBL_0:15>, %v_2, jumpTarget: LBL_0:15, value: %v_2)

block #5 (out: 7,8): LBL_5:-1
  0:  %v_5 := call_0o(self<%self>, callType: VARIABLE, name: object_id, potentiallyRefined: false, flags: 0)
  1:          put_field(self<%self>, %v_5, name: @a, operand1: %self, operand2: %v_5)
  2:          pop_binding
  3:          pop_method_frame
  4:          return(%v_5)

block #8 (out: 7): _GLOBAL_ENSURE_BLOCK__0:-1
  0:  %v_6 := recv_jruby_exc
  1:          pop_binding
  2:          pop_method_frame
  3:          throw(%v_6)

block #7 (out: 8): LBL_6:-1
  0:          return(nil<>)

block #6 (out: 7,8): LBL_0:15
  0:          pop_binding
  1:          pop_method_frame
  2:          return(%v_4)

The defined part of the lookup uses the IS_DEFINED_INSTANCE_VAR runtime helper, which by necessity will try to access the instance vars of the class and check for the given name.

If it is defined, it does a second lookup via get_field.

This should be reduced to a single operation.

[enebo]

@headius yeah I suppose the runtime helper for this could be undef or value then change branch for undef.

[enebo]

What I said is true but this is a lot more work. We generically call into defined logic which is built around producing proper defined string or not. This issue also reflects more than ivars since all ||= accesses which require a defined check have the same issue. So buildGetCondition could be rewritten around UNDEF for all the things and then use a temp for the proper string (and likely something to differentiate defined returning value vs defined returning string). This will make defined logic a little larger but the basic branching logic is the same.

[enebo]

Having implemented a sort of hack where I allow get def to return value vs string I realize how much IR we emit which could just be a lot simpler. We have 2 branches per op or assign and this could just be a single operation. I do not see us ever seeing through an ivar (or anything else we have to retrieve a value like this) so we could just make runtime helper which does the whole thing. In theory, leaving these branches could "magically" get opted away if we could prove something was always true but we just will not have that.

I am looking at this so much because in YARP integration I am looking at op asgn nodes. This is good input.

[enebo]

Derp. Ok without synthetic method this needs one branch still but not two. We cannot evaluate value unless we realize ivar is not there. ok still better than two branches.

[enebo]

heh...I was not seeing improvement because I was using a positive eq branch and that causes an extra jump then I switch to BNE and it was not faster and I thought, wtf. That instr does ruby equality. I guess we need at least another branch instr. I half wonder if we should just add a boolean to each one so we can EQ/NE for specific comparisons. The JIT won't care.

Both methods I have tried had a massive effect on interpreter. Not that it matters but less instrs is less looping in interp.

[headius]

I have wanted to customize many of our conditionals so they only use a boolean and don't need RubyBoolean, but ultimately my goal with this would be to reduce it down to a single ivar lookup that the JIT can cache and optimize; the rest of the logic can be in a helper or indy call site or whatever, but from there it should not need to go back to the ivar table.

[enebo]

I have a branch where my benchmark is 120x faster (lots of repeated @a ||= i). Ultimately, the reduced branching of defined + op or logic is an improvement (very noticeable in interpreter) but the caching behavior due to being in a microbench loop pretty much eliminates any work from happening. So obviously for ||= this is really important for the pattern of use.

I should have a polished PR for tomorrow (btw this does not change the set hash issue at all).

[enebo]

err I should say that the big magic is that defined caches nothing but getfield does. That makes it go from like .6s a loop to 0.02s. The reduction of the extra branching and double grab on interp is 4-6x faster by removing the uncached grab and reducing the amount of instructions interpreted.

[headius]

@enebo That sounds excellent! And with indy inlining the variable access, it should be no more expensive to conditionally access a field than to directly access it.

[headius]

Fixed by #7896. I'll file a separate issue for caching defined? failures.