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.

Sign up for GitHub

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

Jump to bottom

Extra jumps + instrs for exceptions #3354

Open
enebo opened this issue Sep 26, 2015 · 4 comments
Open

Extra jumps + instrs for exceptions #3354

enebo opened this issue Sep 26, 2015 · 4 comments
Labels
ir JRuby 9000 performance

Comments

@enebo
Copy link
Member

enebo commented Sep 26, 2015

If we look at a very simple snippet in IR:

jruby -S ast --ir -e 'a = 1; begin; foo; rescue; nil; end'

Here is IR of last normal pass:

------ Rescue block map ------
BB 4 --> BB 6
BB 6 --> BB 10
BB 7 --> BB 10
BB 8 --> BB 10

:Instructions
2015-09-26T10:48:02.173-05:00: BasicCompilerPassListener: Finished Dead Code Elimination on scope in 1ms.
2015-09-26T10:48:02.173-05:00: BasicCompilerPassListener: Starting Add Local Variable Load/Store Instructions on scope Script: file: -SCRIPT_BODY -[-:0]
2015-09-26T10:48:02.239-05:00: BasicCompilerPassListener: 
Graph:
BB [1:LBL_10:-1]:>[2,12]
BB [2:LBL_11:-1]:>[4], <[1]
BB [4:LBL_4:10]:>[5,6], <[2]
BB [5:LBL_6:17]:>[11], <[4]
BB [6:LBL_5:21]:>[7,8,10], <[4]
BB [7:LBL_7:26]:>[10,12], <[6]
BB [8:LBL_8:28]:>[9,10], <[6]
BB [9:LBL_9:31]:>[11], <[8]
BB [10:LBL_3:36]:>[12], <[6,7,8]
BB [11:LBL_2:40]:>[12], <[5,9]
BB [12:LBL_12:-1]:<[1,7,10,11]

2015-09-26T10:48:02.505-05:00: BasicCompilerPassListener: 
Instructions[-#0#,IRScriptBody,EnsureTempsAssigned]:
BB [1:LBL_10:-1]
    %v_4 = copy(nil)
    %v_5 = copy(%v_4)
    %v_3 = copy(%v_4)
    %v_8 = copy(%v_4)
    %v_9 = copy(%v_4)
    %v_6 = copy(%v_4)
    %v_7 = copy(%v_4)
    %t_a_11 = copy(%v_4)
BB [2:LBL_11:-1]
    [DEAD]%self = recv_self()
    line_num(;n: 0)
    %t_a_11 = copy(Fixnum:1)
    %v_3 = get_global_var($!)
BB [4:LBL_4:10]
    toggle_backtrace(;true)
    binding_store(%t_a_11, a(0:0) ;scope_name: -)
    %v_5 = call_0o(%self ;n:foo, t:VA, cl:false)
    %v_4 = copy(%v_5)
    put_global_var($!, %v_3)
BB [5:LBL_6:17]
    jump(LBL_2:40)
BB [6:LBL_5:21]
    %v_6 = recv_ruby_exc()
    %v_7 = inheritance_search_const(<Class:Object> ;name: StandardError, no_priv: false)
    %v_8 = rescue_eqq(%v_7, %v_6)
    b_true(LBL_8:28, %v_8)
BB [7:LBL_7:26]
    throw(%v_6)
BB [8:LBL_8:28]
    %v_4 = copy(nil)
    put_global_var($!, %v_3)
BB [9:LBL_9:31]
    jump(LBL_2:40)
BB [10:LBL_3:36]
    %v_9 = recv_jruby_exc()
    %v_10 = runtime_helper(%v_9, %v_3 ;method: RESTORE_EXCEPTION_VAR)
    throw(%v_9)
BB [11:LBL_2:40]
    return(%v_4)
BB [12:LBL_12:-1]

Notice in an ideal world BB6 could fall through to BB8 and BB7 could be deleted and the b_true in BB6 could become a b_false and go straight to BB 10. This would end up eliminating an extra BB and also not force us to throw an extra time. @subbuss said he thought perhaps this can be post pass on CFG construction since it can happen in more conditions than this single rescue form.
@subbuss
Copy link
Contributor

subbuss commented Sep 26, 2015

I think even without any of the branch condition changes, a throw in a scope that gets caught unconditionally by a catch in the same scope could be replaced with a jump ... but now that I said it, I think it will require BB 10 to be split into 2 since recv_jruby_exc in BB10 will no longer apply without the throw. Anyway recording this for posterity if this ever shows up as a perf issue.

@enebo
Copy link
Member Author

enebo commented Sep 26, 2015

@subbuss I think perhaps we should also consider the rescues above (it will not end up changing your conclusion about splitting 10 unfortunately):

BB 6 --> BB 10
BB 7 --> BB 10
BB 8 --> BB 10

The optimization would be to remove 7 so then only 6 and 8 can throw to 10. Can 8 really throw? It may be worth it to consider this FIXME you made:

 // SSS FIXME: Not all global variable sets can throw exceptions.  Should we split this
    // operation into two different operations?  Those that can throw exceptions and those
    // that cannot.  But, for now, this should be good enough

I do wonder how much f_can_raise_exception is defeating some optimizations. I guess calls are so frequent in Ruby code perhaps it does not matter a ton, but in this case I do not think $! need to worry about throwing. With potentially a set_global_safe instr we can eliminate 6 from needing a rescue to 10.

Secondly, I think we could consider that we might have also uncovered a semantic bug in 6. Consider:

mri22 -e 'Object.send(:remove_const, :StandardError); a = foo rescue 1; p a'

Note in IR we end up exposing %undefined and in MRI it works. fwiw, NO ONE will be removing StandardError but I think we should be getting a pre-saved version of StandardError for this case (oh no another instr :) ). The only other instr which can raise is rescue_eqq. I proved to myself that we can actually raise in this one:

mri22 -e 'class StandardError; class << self; def ===(o); raise ArgumentError.new "boooo"; end; end; end; foo rescue nil'

This is a bummer and I guess means we have to split 10. In spite of all this work to realize 10 needs to be split I found a bug in 6 and I think we need to review constants we retrieve to see how many cannot be overriden in user code. Retrieving those with a new instr could also be a perf gain (for that matter %StandardError would not be a bad idea).

@subbuss
Copy link
Contributor

subbuss commented Sep 27, 2015

Reg. StandardError, yes, it is a known issue -- see this FIXME from IRBuilder. :-)

            // SSS FIXME:
            // rescue => e AND rescue implicitly EQQ the exception object with StandardError
            // We generate explicit IR for this test here.  But, this can lead to inconsistent
            // behavior (when compared to MRI) in certain scenarios.  See example:
            //
            //   self.class.const_set(:StandardError, 1)
            //   begin; raise TypeError.new; rescue; puts "AHA"; end
            //
            // MRI rescues the error, but we will raise an exception because of reassignment
            // of StandardError.  I am ignoring this for now and treating this as undefined behavior.
            //
            // Solution: Create a 'StandardError' operand type to eliminate this.

@enebo
Copy link
Member Author

enebo commented Sep 27, 2015

@subbuss haha...IR knows all problems if you read the source long enough :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ir JRuby 9000 performance
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@enebo @subbuss