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

Skip block-to-proc conversion for explicit block syntax when possible #2562

Merged
merged 3 commits into from Feb 6, 2015

Conversation

Projects
None yet
3 participants
@sgrif
Copy link
Contributor

sgrif commented Feb 4, 2015

If an explicit block is captured as a proc, but its only usage is
converting it back to a block and passing it through, we can skip the
conversion entirely, and just pass through the original block.

This pass looks for all explicit block conversions, and checks if
they're ever used in any context besides being passed as a block. If
not, we rewrite all uses of the proc to pass the block through instead.
The dead code elimination will remove the proc as an unused variable.

It may be possible to implement this without adding another compiler
pass, but I had trouble finding a place where it fit nicely, especially
since it needs to run before the LiveVariableAnalysis.

I believe the same optimization could occur to skip array allocation for splatted args, and hash allocation for double splatted args, in this same compiler pass.

Given the following Ruby code:

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

The instruction sequence before was:

%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)
%v_1 = call(FUNCTIONAL, bar, %self, [], &%t_block_2)
return(%v_1)

With this patch, the instruction sequence changes to:

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

Which gives a healthy boost to performance on any code which passes
blocks through explicitly.

@sgrif sgrif force-pushed the sgrif:sg-no-block-conversion-cost branch from 15f11e0 to 839c35e Feb 4, 2015

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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?

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

I'll update once I get into the office

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

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

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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)
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

@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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

@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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

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 enebo added this to the 9.0.0.0.pre2 milestone Feb 5, 2015

@enebo enebo added the ir label Feb 5, 2015

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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

@sgrif sgrif force-pushed the sgrif:sg-no-block-conversion-cost branch from 54757b7 to 9348c0c Feb 5, 2015

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

Or sgrif@9348c0c rather

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

@sgrif yeah. That's what I wanted.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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

Skip block-to-proc conversion for explicit block syntax when possible
If an explicit block is captured as a proc, but its only usage is
converting it back to a block and passing it through, we can skip the
conversion entirely, and just pass through the original block.

This pass looks for all explicit block conversions, and checks if
they're ever used in any context besides being passed as a block. If
not, we rewrite all uses of the proc to pass the block through instead.
The dead code elimination will remove the proc as an unused variable.

It may be possible to implement this without adding another compiler
pass, but I had trouble finding a place where it fit nicely, especially
since it needs to run before the LiveVariableAnalysis.

Given the following Ruby code:

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

The instruction sequence before was:

```
%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)
%v_1 = call(FUNCTIONAL, bar, %self, [], &%t_block_2)
return(%v_1)
```

With this patch, the instruction sequence changes to:

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

Which gives a healthy boost to performance on any code which passes
blocks through explicitly.

@sgrif sgrif force-pushed the sgrif:sg-no-block-conversion-cost branch from 9348c0c to ea19e3c Feb 5, 2015

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

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

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

@sgrif I wonder if there is still a problem:

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

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

@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?

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

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 :)

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 5, 2015

@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 :)

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 5, 2015

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).

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Feb 6, 2015

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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

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).

Skip block-to-proc conversion for explicit block syntax when possible
If an explicit block is captured as a proc, but its only usage is
converting it back to a block and passing it through, we can skip the
conversion entirely, and just pass through the original block.

This pass looks for all explicit block conversions, and checks if
they're ever used in any context besides being passed as a block. If
not, we rewrite the `ReifyProcInstr` to just copy the block, instead.

Given the following Ruby code:

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

The instruction sequence before was:

```
%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)
%v_1 = call(FUNCTIONAL, bar, %self, [], &%t_block_2)
return(%v_1)
```

With this patch, the instruction sequence changes to:

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

Which gives a healthy boost to performance on any code which passes
blocks through explicitly.
@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

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

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

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

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 6, 2015

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

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

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
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 6, 2015

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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

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

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 6, 2015

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)
@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

Jinx, you owe me a coke.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

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.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

We should be good now

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 6, 2015

Great. We will wait for ci seal of approval now

enebo added a commit that referenced this pull request Feb 6, 2015

Merge pull request #2562 from sgrif/sg-no-block-conversion-cost
Skip block-to-proc conversion for explicit block syntax when possible.

@enebo enebo merged commit f8a868c into jruby:master Feb 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 6, 2015

@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.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 6, 2015

And thanks again for digging into this.

@sgrif sgrif deleted the sgrif:sg-no-block-conversion-cost branch Feb 6, 2015

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Feb 6, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.