Thanks for the review. Yeah, I figured there were a few edge cases that I needed to handle better. The second case is tough, I didn't even consider it. I think allocating a new temp makes perfect sense. It seems like we'd want to have this be the very first compiler pass that gets run then, right? That way the new temp var can get optimized away when possible?

Also what's the best place for me to add some explicit test cases for these edge cases?

Replacing the reify with a %new_tmp = copy(%v1) would definitely solve the second issue. I agree that simplest for first problem is to not perform the optimization on scopes with BINDING_HAS_ESCAPED and if it has no nested scopes. Nested scopes are not insurmountable but it becomes more sophisticated to cope with.

Yeah, I definitely think we just skip it with nested scopes. The kind of code that will benefit most from this is much more likely to fall into the simplest case

I'll update once I get into the office

Also it would be cool to see a small bench showing the benefit too. Numbers are fun :)

Sure. I ran them showing 20% improvement for the simple case last night, I'll post the full benchmark though

I've updated to account for the two issues raised. I still need to refactor the code a bit. Right now the code is assuming that the only place the result of LoadImplicitClosureInstr can be used is in ClosureAcceptingInstr. If that's incorrect, I can change it to rename all uses of the temp until it's assigned to something else.

The instructions generated for:

def foo(&block)
  p 1+2+3
  bar(&block)
end

is now

    %self = recv_self
    check_arity req: 0, opt: 0, *r: -1, kw: false, **r: -1
    %v_3 = load_implicit_closure
    line_num(1)
    %v_1 = call_1f(NORMAL, +, Fixnum:1, [Fixnum:2]){1F}
    %v_2 = call_1f(NORMAL, +, %v_1, [Fixnum:3]){1F}
    noresult_call_1o(FUNCTIONAL, p, %self, [%v_2])
    line_num(2)
    %v_2 = call(FUNCTIONAL, bar, %self, [], &%v_3)
    return(%v_2)

@sgrif I was originally hoping to not have this be a pass because it will slow startup time down to run this on everything but @subbuss mentioned a simple mitigating solution. We can create an new IRFlag to mark scopes which contain block pass args (&foo) and only run this optimization pass for those scopes. That will eliminate this running in 95%+ of the scopes encountered.

@enebo Absolutely. I am by no means coupled to keeping this as a separate pass. It just made getting the initial implementation done easier (given my unfamiliarity with the code base). I think the flag is a great solution though, I'll update to implement it.

Here's the benchmarks you asked for.

Script:

def baz(x)
  x + yield
end

def bar(&block)
  baz(1, &block)
end

def foo(&block)
  bar(&block)
end

require "benchmark/ips"

Benchmark.ips do |bm|
  bm.report { foo { 1 } }
end

Before:

Calculating -------------------------------------
                        99.181k i/100ms
-------------------------------------------------
                          5.197M (± 9.1%) i/s -     25.787M

After:

Calculating -------------------------------------
                       116.413k i/100ms
-------------------------------------------------
                          6.230M (± 8.3%) i/s -     30.849M

Cool. I remember this started by your observation that a literal block was faster than a block pass var passed through. Are we the same speed for both now (I know you mentioned MRI but I am just curious)?
This question has nothing to do with the PR :)

20% is nothing to sneeze at.

I'll compare the two. I was actually hoping to show some real world comparisons on the code in AR that was extremely affected by this (on MRI), but last I checked the JDBC adapter still doesn't support 4.2.

I've updated to skip this pass if the scope doesn't receive closure args. Turns out that flag was already there for us to use. Let me know if there's any other changes I need to make, if not I'll squash this down.

Here's the comparison between the two forms. (script)

Results on my branch:

Calculating -------------------------------------
              &block   104.337k i/100ms
              manual    93.058k i/100ms
-------------------------------------------------
              &block      6.264M (± 7.4%) i/s -     31.092M
              manual      2.727M (± 4.9%) i/s -     13.586M

Comparison:
              &block:  6263991.9 i/s
              manual:  2727019.1 i/s - 2.30x slower

I checked on master too, and it actually looks like the &block form was already faster than the manual form on jruby master. (For reference &block is 2x slower than manual on MRI). Also interesting is that it's slower than the manual form during the warmup phase. I'm guessing the JIT has a lot to do with it?

For reference here's the other results:

jruby master:

Calculating -------------------------------------
              &block    80.543k i/100ms
              manual    90.985k i/100ms
-------------------------------------------------
              &block      5.028M (±11.3%) i/s -     24.727M
              manual      2.724M (± 7.2%) i/s -     13.557M

Comparison:
              &block:  5028152.0 i/s
              manual:  2723964.1 i/s - 1.85x slower

MRI 2.2:

Calculating -------------------------------------
              &block    67.452k i/100ms
              manual    93.462k i/100ms
-------------------------------------------------
              &block      1.316M (± 6.3%) i/s -      6.610M
              manual      2.496M (± 5.1%) i/s -     12.524M

Comparison:
              manual:  2495676.5 i/s
              &block:  1316196.7 i/s - 1.90x slower

So only one more change and this looks great. Can you not add it as a pass if that flag is set instead of running the pass and having it decide to not operate on it?

Startup time is a biggest risk in 9k right now and I noticed this small amount of setup logic to starting a pass is measurable :|

@enebo Just confirming that sgrif@54757b7 is what you meant by that?

Or sgrif@9348c0c rather

@sgrif yeah. That's what I wanted.

Cool. I'll squash into a single commit. Were there any explicit tests I should add for this?

No tests for this. Behaviorally I am waiting for CI to clear this though.

@sgrif I wonder if there is still a problem:

def(&b)
  b = Proc.new
  foo(&b)
end

I'm not sure. I'm definitely replacing the local with a temp, but I'm not sure that's going to hurt anything, since the local will always get eliminated anyway (except for in cases where this pass wouldn't run). Are you specifically referring to the behavior of Proc.new with regards to the implicit block, or are you saying this would have the same problem?

def foo(&block)
  block = method_returning_proc
  bar(&block)
end

@sgrif I should not have used Proc.new. I just mean if you end up reassigning variable block without some re-assignment detection you may pass wrong version of block?

As far as I can tell, it's fine. The only side effect is that the local variable will get replaced with a temp variable instead, which I don't think is a problem. It still assigns everything to the right variable no problem.

def foo(&block)
  block = method_returning_proc(&block)
  bar(&block)
end

    %self = recv_self
    check_arity req: 0, opt: 0, *r: -1, kw: false, **r: -1
    %v_2 = load_implicit_closure
    line_num(1)
    %v_2 = call(FUNCTIONAL, method_returning_proc, %self, [], &%v_2)
    line_num(2)
    %v_1 = call(FUNCTIONAL, bar, %self, [], &%v_2)
    return(%v_1)

on master:

    %self = recv_self
    check_arity req: 0, opt: 0, *r: -1, kw: false, **r: -1
    %v_1 = load_implicit_closure
    %t_block_2 = reify_closure(%v_1)
    line_num(1)
    %t_block_2 = call(FUNCTIONAL, method_returning_proc, %self, [], &%t_block_2)
    line_num(2)
    %v_1 = call(FUNCTIONAL, bar, %self, [], &%t_block_2)
    return(%v_1

So it looks like the only time the results would be different would be some case where the "Optimize Dynamic Scopes" pass doesn't replace the local variable with a temporary one. But I don't think there are any cases where that would happen and we run this pass.

So in looking I cannot find a scenario which breaks. My notes ( @subbuss will also be looking tonight):

1. If you redefine block in a non-block way then the opt does not happen so:

def foo(&block)
  block = something if something_else
  bar &block
end

will not apply this because block is used as an ordinary variable. Good.

1. The variable replacement seems to always work because the new temp var is created after the method has been completely built. This means it will never pick an existing temporary variable and will not trip over another use of temporary variable. Also Good.

So I am back to thinking this is working :)

Yeah, sounds like we're on the same page on that, then. I'm certainly happy to just skip the optimization if the variable is assigned if you guys would be more comfortable with that, but it seems to work regardless.

@sgrif I am about done for today but with your patch applied I cannot run gem list. I reduced it to this case:

def bar
  yield
end

def foo(&block)
  bar &block if block_given?
end

foo

So there is still a somewhat simple error to correct. We will get there :)

Cool, I'm heading out too. I'll dig into it tomorrow.

On Thu, Feb 5, 2015, 4:19 PM Thomas E Enebo notifications@github.com
wrote:

@sgrif https://github.com/sgrif I am about done for today but with your
patch applied I cannot run gem list. I reduced it to this case:

def bar
yieldend
def foo(&block)
bar &block if block_given?end

foo

So there is still a somewhat simple error to correct. We will get there :)

—
Reply to this email directly or view it on GitHub
#2562 (comment).

Try this:

def foo(&block)
  block = nil
  p 1+2+3
  bar(&block)
end

You get:

        %self = recv_self
        check_arity req: 0, opt: 0, *r: -1, kw: false, **r: -1
        %v_3 = load_implicit_closure
        line_num(7)
        line_num(8)
        %v_1 = call_1f(NORMAL, +, Fixnum:1, [Fixnum:2]){1F}
        %v_2 = call_1f(NORMAL, +, %v_1, [Fixnum:3]){1F}
        noresult_call_1o(FUNCTIONAL, p, %self, [%v_2])
        line_num(9)
        %v_2 = call(FUNCTIONAL, bar, %self, [], &%v_3)

So, you've effectively lost the "block = nil" initialization.

Arbitrary variable renaming is tricky if the scope is not in SSA form or if you are not using use-def information. You have to make sure intermediate instructions don't clobber the variable that is being renamed downstream.

I think the safe solution is to use what Tom proposed earlier which also significantly simplifies this code, and that is to replace the "v = reify_closure(block)" to "v = copy(block)" which just passes through the block unmodified (which is the intention of your optimization) without having to deal with any of the use-def hazards. And, later passes could optimize the copy and even if gets to bytecode, the JVM will trivially take care of the copy.

I should have thought of this yesterday, but better late than never :-) .. Take a look at passes/LocalOptimizationPass.java and how it has to deal with these same use-def issues, and that pass only handles things within a basic-block. That pass effectively replaces a simplifiable instruction with a copy (which is what you are doing as well). Since your pass needs global information (i.e. no instr. within the scope uses the block in any other way), we cannot implement the simplification code in LocalOptimizationPass. However, since 'scope' is passed into simplifyAndGetResult method of Instr.java, ReifyInstruction.java could implement the global analysis as part of its implementation of simplifyAndGetResult and everything should work fine ... However, that feels a bit hacky at this time (for some reason that I cannot yet quite articulate) .. so, let us leave this as a separate pass right now and we can revisit how to merge it in later.

We can chat more on IRC tomorrow if required.

Thanks, subbuss. I'll dig into this further tomorrow, and ping you on IRC
if needed.

On Thu, Feb 5, 2015, 10:40 PM subbuss notifications@github.com wrote:

Try this:

def foo(&block)
block = nil
p 1+2+3
bar(&block)
end

You get:

    %self = recv_self
    check_arity req: 0, opt: 0, *r: -1, kw: false, **r: -1
    %v_3 = load_implicit_closure
    line_num(7)
    line_num(8)
    %v_1 = call_1f(NORMAL, +, Fixnum:1, [Fixnum:2]){1F}
    %v_2 = call_1f(NORMAL, +, %v_1, [Fixnum:3]){1F}
    noresult_call_1o(FUNCTIONAL, p, %self, [%v_2])
    line_num(9)
    %v_2 = call(FUNCTIONAL, bar, %self, [], &%v_3)

So, you've effectively lost the "block = nil" initialization.

Arbitrary variable renaming is tricky if the scope is not in SSA form or
if you are not using use-def information. You have to make sure
intermediate instructions don't clobber the variable that is being renamed
downstream.

I think the safe solution is to use what Tom proposed earlier which also
significantly simplifies this code, and that is to replace the "v =
reify_closure(block)" to "v = copy(block)" which just passes through the
block unmodified (which is the intention of your optimization) without
having to deal with any of the use-def hazards. And, later passes could
optimize the copy and even if gets to bytecode, the JVM will trivially take
care of the copy.

I should have thought of this yesterday, but better late than never :-) ..
Take a look at passes/LocalOptimizationPass.java and how it has to deal
with these same use-def issues, and that pass only handles things within a
basic-block. That pass effectively replaces a simplifiable instruction with
a copy (which is what you are doing as well). Since your pass needs global
information (i.e. no instr. within the scope uses the block in any other
way), we cannot implement the simplification code in LocalOptimizationPass.
However, since 'scope' is passed into simplifyAndGetResult method of
Instr.java, ReifyInstruction.java could implement the global analysis as
part of its implementation of simplifyAndGetResult and everything should
work fine ... However, that feels a bit hacky at this time (for some reason
that I cannot yet quite articulate) .. so, let us leave this as a separate
pass right now and we can revisit how to merge it in later.

We can chat more on IRC tomorrow if required.

—
Reply to this email directly or view it on GitHub
#2562 (comment).

I've updated to use a copy instruction, rather than renaming the variable

gem list still isn't working. I'll keep digging on what the problem is.

The problem is that &block need not receive a block so it instead gets assigned to nil if no block is given.

I'm having trouble isolating when that's an issue, though. This code works fine.

def bar
  yield
end

def foo(&block)
  bar &block if block_given?
end

foo

With your changes that now runs properly. I thought it minimized the issue in optparse but apparently it is more complex. The error now complains about Block and IRubyObject and before it was casting between Block and RubyNil.

Yeah it looks like there is some form of a conditional that it isn't detecting the block being used. I'm digging further now.

It definitely appears to be OptionParser#accept which is the problem

It is happening here 'executing b_false(LBL_6:33, %t_block_8)'

    #
    # See OptionParser.accept.
    #
    def accept(t, pat = /.*/m, &block)
      if pat
        pat.respond_to?(:match) or
          raise TypeError, "has no `match'", ParseError.filter_backtrace(caller(2))
      else
        pat = t if t.respond_to?(:match)
      end
      unless block
        block = pat.method(:convert).to_proc if pat.respond_to?(:convert)
      end
      @atype[t] = [pat, block]
    end

with IR:

BB [1:LBL_10:0]
    push_frame()
BB [2:LBL_11:1]
    %self = recv_self()
    check_arity(;req: 1, opt: 1, *r: -1, kw: false)
    %t_t_6 = recv_pre_reqd_arg()
    %t_pat_7 = recv_opt_arg(;index:0, req: 1, pre: 1)
    bne(LBL_0:6, %t_pat_7, %undefined)
BB [3:LBL_12:5]
    %t_pat_7 = copy(RE:|.*|RegexpOptions(kcode: NONE, kcodeDefault, multiline))
BB [4:LBL_0:6]
    %v_1 = load_implicit_closure()
    %t_block_8 = copy(%v_1)
    line_num(;n: 607)
    b_false(LBL_1:23, %t_pat_7)
BB [5:LBL_13:10]
    line_num(;n: 608)
    %v_1 = call_1o(%t_pat_7, :'match' ;n:respond_to?, t:NO, cl:false)
    b_true(LBL_3:18, %v_1)
BB [6:LBL_14:13]
    %v_2 = search_const(scope<1> ;name: TypeError, no_priv: false)
    %v_3 = search_const(scope<1> ;name: ParseError, no_priv: false)
    %v_4 = call_1f(%self, Fixnum:2 ;n:caller, t:FU, cl:false)
    %v_5 = call_1o(%v_3, %v_4 ;n:filter_backtrace, t:NO, cl:false)
    %v_4 = call(%self, %v_2, "has no `match'", %v_5 ;n:raise, t:FU, cl:false)
BB [7:LBL_3:18]
    jump(LBL_2:30)
BB [8:LBL_1:23]
    line_num(;n: 611)
    %v_4 = call_1o(%t_t_6, :'match' ;n:respond_to?, t:NO, cl:false)
    b_false(LBL_4:29, %v_4)
BB [9:LBL_15:26]
    %t_pat_7 = copy(%t_t_6)
    %v_4 = copy(%t_t_6)
    jump(LBL_5:30)
BB [10:LBL_4:29]
    %v_4 = copy(nil)
BB [11:LBL_5:30]
BB [12:LBL_2:30]
    line_num(;n: 613)
    b_false(LBL_6:33, %t_block_8)
BB [13:LBL_16:32]
    jump(LBL_7:40)
BB [14:LBL_6:33]
    line_num(;n: 614)
    %v_4 = call_1o(%t_pat_7, :'convert' ;n:respond_to?, t:NO, cl:false)
    b_false(LBL_8:40, %v_4)
BB [15:LBL_17:36]
    %v_4 = call_1o(%t_pat_7, :'convert' ;n:method, t:NO, cl:false)
    %v_1 = call_0o(%v_4 ;n:to_proc, t:NO, cl:false)
    %t_block_8 = copy(%v_1)
    jump(LBL_9:40)
BB [16:LBL_8:40]
BB [17:LBL_9:40]
BB [18:LBL_7:40]
    line_num(;n: 616)
    %v_1 = get_field(%self ;name: @atype)
    %v_4 = copy(Array:[%t_pat_7, %t_block_8])
    attr_assign(%v_1, %t_t_6, %v_4 ;n:[]=, t:UN, cl:false)
    pop_frame()
    return(%v_4)
BB [19:LBL_18:47]
    return(nil)
BB [20:_GLOBAL_ENSURE_BLOCK__0:19]
    %v_9 = recv_jruby_exc()
    pop_frame()
    throw(%v_9)

Jinx, you owe me a coke.

Ok, I think I see the problem. I'm doing the tracking & replacing per basic block, but I need to do it for the whole scope.

We should be good now

Great. We will wait for ci seal of approval now

@sgrif I will make some style tweaks later today for code style and I will refer to the commit here. I did not want to pester you with those and would rather just show them.

And thanks again for digging into this.

❤️ 💛 💚 💙 💜 ❤️ 💛 💚 💙 💜 ❤️ 💛 💚 💙 💜 ❤️ 💛 💚 💙 💜 ❤️ 💛 💚 💙 💜 ❤️ 💛 💚 💙 💜