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

Call rb_ivar_get and rb_ivar_set instead of exiting for many ivars #7335

Merged

Conversation

eileencodes
Copy link
Contributor

Previously, when we have a lot of ivars defined, we would exit via jit_chain_guard for megamorphic ivars. Now if we have more than the max depth of ivars we can call rb_ivar_get and rb_ivar_set respectively.

Using the following script:

class A
  def initialize
    @a = 1
  end

  def a
    @a
  end
end

N = 30
N.times do |i|
  eval <<-eorb
class A#{i} < A
  def initialize
    @a#{i} = 1
    super
  end
end
  eorb
end

klasses = N.times.map { Object.const_get(:"A#{_1}") }

1000.times do
  klasses.each do |k|
    k.new.a
  end
end

Exits before this change show exits for getinstancevariable and setinstancevariable:

***YJIT: Printing YJIT statistics on exit***
method call exit reasons:
    klass_megamorphic:     24,975 (100.0%)
invokeblock exit reasons:
    (all relevant counters are zero)
invokesuper exit reasons:
    (all relevant counters are zero)
leave exit reasons:
    interp_return:     26,948 (100.0%)
     se_interrupt:          1 ( 0.0%)
getblockparamproxy exit reasons:
    (all relevant counters are zero)
getinstancevariable exit reasons:
    megamorphic:     13,986 (100.0%)
setinstancevariable exit reasons:
    megamorphic:     19,980 (100.0%)
opt_aref exit reasons:
    (all relevant counters are zero)
expandarray exit reasons:
    (all relevant counters are zero)
opt_getinlinecache exit reasons:
    (all relevant counters are zero)
invalidation reasons:
    (all relevant counters are zero)
num_send:                    155,823
num_send_known_class:              0 ( 0.0%)
num_send_polymorphic:        119,880 (76.9%)
bindings_allocations:              0
bindings_set:                      0
compiled_iseq_count:              36
compiled_block_count:            158
compiled_branch_count:           240
block_next_count:                 10
defer_count:                      70
freed_iseq_count:                  0
invalidation_count:                0
constant_state_bumps:              0
inline_code_size:             29,216
outlined_code_size:           27,948
freed_code_size:                   0
code_region_size:             65,536
live_context_size:             8,322
live_context_count:              219
live_page_count:                   4
freed_page_count:                  0
code_gc_count:                     0
num_gc_obj_refs:                 130
object_shape_count:              295
side_exit_count:              58,942
total_exit_count:             85,890
yjit_insns_count:          1,023,581
avg_len_in_yjit:                11.2
Top-4 most frequent exit ops (100.0% of exits):
    opt_send_without_block:     24,975 (42.4%)
       setinstancevariable:     19,980 (33.9%)
       getinstancevariable:     13,986 (23.7%)
		     leave:          1 ( 0.0%)

Exits after this change show we have no exits for getinstanevariable and setinstancevariable.

***YJIT: Printing YJIT statistics on exit***
method call exit reasons:
    klass_megamorphic:     24,975 (100.0%)
invokeblock exit reasons:
    (all relevant counters are zero)
invokesuper exit reasons:
    (all relevant counters are zero)
leave exit reasons:
    interp_return:     60,912 (100.0%)
     se_interrupt:          3 ( 0.0%)
getblockparamproxy exit reasons:
    (all relevant counters are zero)
getinstancevariable exit reasons:
    (all relevant counters are zero)
setinstancevariable exit reasons:
    (all relevant counters are zero)
opt_aref exit reasons:
    (all relevant counters are zero)
expandarray exit reasons:
    (all relevant counters are zero)
opt_getinlinecache exit reasons:
    (all relevant counters are zero)
invalidation reasons:
    (all relevant counters are zero)
num_send:                    155,823
num_send_known_class:              0 ( 0.0%)
num_send_polymorphic:        119,880 (76.9%)
bindings_allocations:              0
bindings_set:                      0
compiled_iseq_count:              36
compiled_block_count:            179
compiled_branch_count:           240
block_next_count:                 11
defer_count:                      70
freed_iseq_count:                  0
invalidation_count:                0
constant_state_bumps:              0
inline_code_size:             31,032
outlined_code_size:           29,708
freed_code_size:                   0
code_region_size:             65,536
live_context_size:             8,360
live_context_count:              220
live_page_count:                   4
freed_page_count:                  0
code_gc_count:                     0
num_gc_obj_refs:                 130
object_shape_count:              295
side_exit_count:              24,978
total_exit_count:             85,890
yjit_insns_count:          1,076,966
avg_len_in_yjit:                12.2
Top-2 most frequent exit ops (100.0% of exits):
    opt_send_without_block:     24,975 (100.0%)
		     leave:          3 ( 0.0%)

@eileencodes
Copy link
Contributor Author

cc/ @tenderlove

@matzbot matzbot requested a review from a team February 17, 2023 20:05
@XrXr
Copy link
Member

XrXr commented Feb 17, 2023

Looks like for gets this conflicts with #7334

@eileencodes
Copy link
Contributor Author

Oh oops 🤦🏼‍♀️

@maximecb
Copy link
Contributor

Kokubun did only implement the change for getivar though, so worth looking at his PR and implementing the change for set.

@k0kubun
Copy link
Member

k0kubun commented Feb 17, 2023

Yeah, I worked only on setivar because SFR was like:

getinstancevariable exit reasons:
    megamorphic: 85,342,998 (100.0%)
setinstancevariable exit reasons:
    megamorphic:          3 (100.0%)

but supporting setivar in this PR seems helpful for other benchmarks, looking at your description.

I confirmed that the CI of my PR was passing and merged it. Please rebase your branch against master 🙇

@@ -1951,7 +1951,7 @@ fn gen_get_ivar(
// Eventually, we can encode whether an object is T_OBJECT or not
// inside object shapes.
// too-complex shapes can't use index access, so we use rb_ivar_get for them too.
if !receiver_t_object || uses_custom_allocator || comptime_receiver.shape_too_complex() {
if !receiver_t_object || uses_custom_allocator || comptime_receiver.shape_too_complex() || (ctx.get_chain_depth() as i32) >= max_chain_depth {
Copy link
Member

@k0kubun k0kubun Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #7334, I generated this at == max_chain_depth - 1 instead of == max_chain_depth (or >=) because I thought the C call block should also be counted as part of the number of chains. But if we think about the limit as the number of jit_chain_guard calls, then doing this at == max_chain_depth (or >=) makes sense 👍

Previously, when we have a lot of ivars defined, we would exit via
`jit_chain_guard` for megamorphic ivars. Now if we have more than the
max depth of ivars we can call `rb_ivar_set` instead of exiting.

Using the following script:

```ruby
class A
  def initialize
    @A = 1
  end

  def a
    @A
  end
end

N = 30
N.times do |i|
  eval <<-eorb
class A#{i} < A
  def initialize
    @A#{i} = 1
    super
  end
end
  eorb
end

klasses = N.times.map { Object.const_get(:"A#{_1}") }

1000.times do
  klasses.each do |k|
    k.new.a
  end
end
```

Exits before this change show exits for `setinstancevariable`:

```
***YJIT: Printing YJIT statistics on exit***
method call exit reasons:
    klass_megamorphic:     24,975 (100.0%)
invokeblock exit reasons:
    (all relevant counters are zero)
invokesuper exit reasons:
    (all relevant counters are zero)
leave exit reasons:
    interp_return:     26,948 (100.0%)
     se_interrupt:          1 ( 0.0%)
getblockparamproxy exit reasons:
    (all relevant counters are zero)
getinstancevariable exit reasons:
    megamorphic:     13,986 (100.0%)
setinstancevariable exit reasons:
    megamorphic:     19,980 (100.0%)
opt_aref exit reasons:
    (all relevant counters are zero)
expandarray exit reasons:
    (all relevant counters are zero)
opt_getinlinecache exit reasons:
    (all relevant counters are zero)
invalidation reasons:
    (all relevant counters are zero)
num_send:                    155,823
num_send_known_class:              0 ( 0.0%)
num_send_polymorphic:        119,880 (76.9%)
bindings_allocations:              0
bindings_set:                      0
compiled_iseq_count:              36
compiled_block_count:            158
compiled_branch_count:           240
block_next_count:                 10
defer_count:                      70
freed_iseq_count:                  0
invalidation_count:                0
constant_state_bumps:              0
inline_code_size:             29,216
outlined_code_size:           27,948
freed_code_size:                   0
code_region_size:             65,536
live_context_size:             8,322
live_context_count:              219
live_page_count:                   4
freed_page_count:                  0
code_gc_count:                     0
num_gc_obj_refs:                 130
object_shape_count:              295
side_exit_count:              58,942
total_exit_count:             85,890
yjit_insns_count:          1,023,581
avg_len_in_yjit:                11.2
Top-4 most frequent exit ops (100.0% of exits):
    opt_send_without_block:     24,975 (42.4%)
       setinstancevariable:     19,980 (33.9%)
       getinstancevariable:     13,986 (23.7%)
		     leave:          1 ( 0.0%)
```

Exits after this change show we have no exits for `setinstancevariable`.

```
***YJIT: Printing YJIT statistics on exit***
method call exit reasons:
    klass_megamorphic:     24,975 (100.0%)
invokeblock exit reasons:
    (all relevant counters are zero)
invokesuper exit reasons:
    (all relevant counters are zero)
leave exit reasons:
    interp_return:     60,912 (100.0%)
     se_interrupt:          3 ( 0.0%)
getblockparamproxy exit reasons:
    (all relevant counters are zero)
getinstancevariable exit reasons:
    (all relevant counters are zero)
setinstancevariable exit reasons:
    (all relevant counters are zero)
opt_aref exit reasons:
    (all relevant counters are zero)
expandarray exit reasons:
    (all relevant counters are zero)
opt_getinlinecache exit reasons:
    (all relevant counters are zero)
invalidation reasons:
    (all relevant counters are zero)
num_send:                    155,823
num_send_known_class:              0 ( 0.0%)
num_send_polymorphic:        119,880 (76.9%)
bindings_allocations:              0
bindings_set:                      0
compiled_iseq_count:              36
compiled_block_count:            179
compiled_branch_count:           240
block_next_count:                 11
defer_count:                      70
freed_iseq_count:                  0
invalidation_count:                0
constant_state_bumps:              0
inline_code_size:             31,032
outlined_code_size:           29,708
freed_code_size:                   0
code_region_size:             65,536
live_context_size:             8,360
live_context_count:              220
live_page_count:                   4
freed_page_count:                  0
code_gc_count:                     0
num_gc_obj_refs:                 130
object_shape_count:              295
side_exit_count:              24,978
total_exit_count:             85,890
yjit_insns_count:          1,076,966
avg_len_in_yjit:                12.2
Top-2 most frequent exit ops (100.0% of exits):
    opt_send_without_block:     24,975 (100.0%)
		     leave:          3 ( 0.0%)
```

Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org>
@eileencodes eileencodes force-pushed the call-rb-ivar-methods-for-too-many-ivars branch from 58d4bb4 to b9e5d0d Compare February 17, 2023 21:19
@tenderlove tenderlove merged commit ae9e1ae into ruby:master Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants