Emit a performance warning when redefining specially optimized methods

[casperisfine]

This makes it easier to notice a dependency is causing interpreter or JIT deoptimization.

Warning[:performance] = true

class String
  def freeze
    super
  end
end

./test.rb:4: warning: Redefining 'String#freeze' disable multiple interpreter and JIT optimizations

[eregon]

./test.rb:4: warning: Redefining 'String#freeze' disable multiple interpreter and JIT optimizations

I would suggest to replace multiple by <nothing> or several or some to avoid confusion.
On my first read I thought it disabled "multiple interpreter" (i.e. MVM) optimizations.
(but maybe I just read too fast too)

Also disable -> disables

[casperisfine]

@eregon done.

[ko1]

@byroot
I don't have strong opinion but could you make a ticket about it and NEWS entry?
I'll confirm it tomorrow's devmeeting by asking matz if the ticket is avaiable.

[casperisfine]

could you make a ticket about it and NEWS entry?

Ah sorry, I went a bit too fast indeed. Here's the ticket and NEWS update:

https://bugs.ruby-lang.org/issues/20429

#10542

[ko1]

Thank you!

[headius]

These should not be tested as part of normal behavioral tests for these methods. This performance warning is specific to CRuby's way of optimizing and other implementations may not have the same limitation. Having these tests be done alongside normal behavioral testing means all implementations must warn for redefinition regardless of whether performance optimizations are affected.

These tests are also not a core behavior of any of these methods, and are a side effect of one implementation. They should be tested separately.

[eregon]

These are CRuby tests, it shouldn't be surprising they test CRuby-specific details.
It's good these new warnings are tested, and where else than test/ruby/test_optimization.rb which already has a list of such intrinsified methods?
It should be very easy for JRuby to define assert_performance_warning as no-op if it doesn't want to emit such warnings or test that there.

FWIW TruffleRuby has very similar warnings although for a slightly different set of methods:
https://github.com/oracle/truffleruby/blob/master/spec/truffle/redefining_optimized_core_methods_spec.rb

These tests are also not a core behavior of any of these methods, and are a side effect of one implementation. They should be tested separately.

The whole file actually only makes sense if these methods are intrinsified. Otherwise there would be no reason to test redefining more than one example method.
"separately" would mean duplicating the list, it seems obvious this is not desirable.

[headius]

It should be very easy for JRuby to define assert_performance_warning

JRuby does not patch tests from MRI. We run them as is without modification and submit patches back when there are MRI specific expectations such as using RubyVM or testing for MRI specific behavior.

Is this warning now considered specified Ruby behavior? If so, should it not be tested in ruby/spec?

If it is not specified behavior, why is it tested alongside the ability to redefine these operators, which is specified behavior?

This test has been running in JRuby's test suite for many years. Adding this MRI-specific expectation caused it to start failing.