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

YJIT: Fallback setivar if the next shape is too complex #8152

Merged
merged 1 commit into from Aug 1, 2023

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Aug 1, 2023

Currently, setivar_too_complex is one of the most frequent exit ops on SFR (e.g. 21.7% overall). This exit happens when it needs to transition to a too-complex shape.

This PR hoists out the logic to get the new_shape_id (as well as other information that is used to get new_shape_id and reused later) and check if it's OBJ_TOO_COMPLEX_SHAPE_ID before generating any code. If it is, it fallbacks to the generic setivar C call.

@k0kubun k0kubun marked this pull request as ready for review August 1, 2023 17:33
@matzbot matzbot requested a review from a team August 1, 2023 17:33
@k0kubun k0kubun merged commit 16b91a3 into ruby:master Aug 1, 2023
90 of 93 checks passed
@k0kubun k0kubun deleted the yjit-complex-setivar branch August 1, 2023 18:43
@casperisfine
Copy link
Contributor

I've witnessed YJIT specific crashes since this was merged.

Here's a relatively small repro script:

require 'bundler/inline'

require 'objspace'
# run with --yjit --yjit-call-threshold=1

gemfile do
  source 'https://rubygems.org'
  gem 'mocha', '2.0.4'
  gem 'minitest', '5.18.1'
end

module MyConfig
end

module OtherModule
end
223.times do |iv|
  MyConfig.instance_variable_set("@_iv_#{iv}", 42)
end

require "minitest/autorun"
require "mocha/minitest"

class Module
  def mocha(instantiate = true)
    if instantiate
      @mocha ||= Mocha::Mockery.instance.mock_impersonating(self)
    else
      defined?(@mocha) ? @mocha : nil
    end
  end
end

class BugTest < Minitest::Test
  def test_mocha
    10.times do
      OtherModule.mocha
    end
    puts ObjectSpace.dump(MyConfig)
    puts ObjectSpace.dump(MyConfig.singleton_class)
    MyConfig.mocha
  end
end

@casperisfine
Copy link
Contributor

Ok, so from what I gathered, the interpreter doesn't run into this, because vm_setivar just bails out when on a T_MODULE or T_CLASS, so we end up in the slow path.

From why I understand of the code, YJIT seem to always use the fast path, even with T_MODULE / T_CLASS hence why it runs into that bug.

casperisfine pushed a commit to Shopify/ruby that referenced this pull request Aug 2, 2023
Followup: ruby#8152

If the receiver is a T_MODULE or T_CLASS and has a lot of
ivars, `get_next_shape_internal` will return `NULL`.
casperisfine pushed a commit to Shopify/ruby that referenced this pull request Aug 2, 2023
Followup: ruby#8152

If the receiver is a T_MODULE or T_CLASS and has a lot of
ivars, `get_next_shape_internal` will return `NULL`.
maximecb pushed a commit that referenced this pull request Aug 2, 2023
Followup: #8152

If the receiver is a T_MODULE or T_CLASS and has a lot of
ivars, `get_next_shape_internal` will return `NULL`.

Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants