Feature #7816

Don't invalidate method caches when defining a new method on a class without subclasses

Added by Charlie Somerville about 1 year ago. Updated 4 months ago.

[ruby-core:52075]
Status:Closed
Priority:Normal
Assignee:Koichi Sasada
Category:core
Target version:next minor

Description

=begin
Attached is a patch that avoids incrementing the VM state version when defining a method and these conditions are true:

  • The method is not a redefinition of an existing method in the same class or any superclass
  • The class has no subclasses
  • The class is not a module

This means that defining singleton methods on objects no longer invalidates every method cache. This will significantly improve performance of code that defines singleton methods at runtime (eg. when using OpenStruct)

In my testing, a fresh Rails app boots about 15% faster (~1.7 sec down to ~1.4 sec).

This controller action can do ~440 requests per second with my patch, compared to ~320 requests per second without my patch.

class HomeController < ApplicationController
def index
OpenStruct.new a: 1, b: 2
render text: "home"
end
end

Of course these numbers will vary between apps, but I think this is a good start in improving the performance of a very common use case in Ruby.
=end

dont-invalidate-method-cache-when-defining-new-singleton-methods.patch Magnifier (4.55 KB) Charlie Somerville, 02/10/2013 02:13 AM

feature-7816-v2.patch Magnifier (3.41 KB) Charlie Somerville, 02/10/2013 10:13 PM

feature-7816-v3.patch Magnifier (3.72 KB) Charlie Somerville, 02/16/2013 06:31 PM

History

#1 Updated by Koichi Sasada about 1 year ago

(2013/02/10 2:13), charliesome (Charlie Somerville) wrote:

In my testing, a fresh Rails app boots about 15% faster (~1.7 sec down to ~1.4 sec).

This controller action can do ~440 requests per second with my patch, compared to ~320 requests per second without my patch.

charliesome++

--
// SASADA Koichi at atdot dot net

#2 Updated by Marc-Andre Lafortune about 1 year ago

  • Subject changed from Don't invalidate method caches when defining a new method on a class without subclasses to Don&#x27;t invalidate method caches when defining a new method on a class without subclasses

Speed boosts sounds awesome, especially the 15% rails bootup time, since the example is a bit contrived.

It would have been great to get this in 2.0.0

I'd split the new API from the patch; personally I'm not convinced of the usefulness of Class#hassubclass? or of RubyVM.stateversion

#3 Updated by Yusuke Endoh about 1 year ago

marcandre (Marc-Andre Lafortune) wrote:

It would have been great to get this in 2.0.0

Do not kamikaze!

Yusuke Endoh mame@tsg.ne.jp

#4 Updated by Charlie Somerville about 1 year ago

marcandre (Marc-Andre Lafortune) wrote:

I'd split the new API from the patch; personally I'm not convinced of the usefulness of Class#hassubclass? or of RubyVM.stateversion

I'm not particularly attached to Class#has_subclass? - I don't care if that stays or goes. I figured that if we have the information, we may as well let the user access it.

I think RubyVM.state_version could be very useful for performance profiling. I work on a Rails app that invalidates the method cache ~200 times per request (mainly due to OpenStruct). I think this patch will lower that number somewhat, but I want to keep this API in so I (and others) can monitor cache invalidations and try to eliminate the remaining invalidations.

#5 Updated by Nobuyoshi Nakada about 1 year ago

=begin
Nice.

My thoughts are:

  • RCLASS_INHERITED flag should go to internal.h.
  • Class#has_subclass? is not only useless but harmful, it mimics users when subclasses are removed.
  • RubyVM.state_version seems useless also, and should be hidden.
  • why rbmethodentry() ignores the cache for an undefined method?
  • what are extra parens around RBTYPEP() in rbmethodentry_make().
  • what's inst in .gitignore. =end

#6 Updated by Charlie Somerville about 1 year ago

Thanks for the feedback nobu.

  • RCLASS_INHERITED flag should go to internal.h.

I think it should stay in include/ruby/ruby.h where all the other flags are defined. This way if someone adds a new class flag, they do not accidentally also pick FL_USER5 because they did not see it already in use.

  • Class#has_subclass? is not only useless but harmful, it mimics users when subclasses are removed.

Fair enough, I'll update my patch and remove it.

  • RubyVM.state_version seems useless also, and should be hidden.

I don't think it is useless. It will be useful for performance tuning. Right now there is no way to tell if the method caches have been cleared, which makes profiling and tuning harder.

  • why rbmethodentry() ignores the cache for an undefined method?

Here is an example:

class Foo
unless method_defined?(:bar)
def bar
end
end
end

If cache entries for undefined methods were not ignored, 'bar' would be defined, but calling it would raise NoMethodError, because the global cache thinks it is not defined.

  • what are extra parens around RBTYPEP() in rbmethodentry_make().

Whoops, this is my mistake.

  • what's inst in .gitignore.

Ditto, I use './configure --prefix=pwd/inst', but I forgot to stash before creating the patch.

#8 Updated by Nobuyoshi Nakada about 1 year ago

charliesome (Charlie Somerville) wrote:

I think it should stay in include/ruby/ruby.h where all the other
flags are defined. This way if someone adds a new class flag, they
do not accidentally also pick FL_USER5 because they did not see it
already in use.

Rather I think other RMODULE_* flags also should go to internal.h.

  • Class#has_subclass? is not only useless but harmful, it mimics users when subclasses are removed.

Fair enough, I'll update my patch and remove it.

Or, add subclass count in rbclassextt.

  • what's inst in .gitignore.

Ditto, I use './configure --prefix=pwd/inst', but I forgot to
stash before creating the patch.

I use dotted directories, e.g., .x86_64-linux.

#9 Updated by Charlie Somerville about 1 year ago

  • File feature-7816-v3.patchMagnifier added
  • Subject changed from Don&#x27;t invalidate method caches when defining a new method on a class without subclasses to Don't invalidate method caches when defining a new method on a class without subclasses

I have updated my patch to not clear the cache when a class is garbage collected.

My reasoning is that if a class is garbage collected, then no objects of that class could possibly exist, so the method cache would never be hit.

#10 Updated by Yura Sokolov about 1 year ago

charliesome (Charlie Somerville) wrote:

I have updated my patch to not clear the cache when a class is garbage collected.

My reasoning is that if a class is garbage collected, then no objects of that class could possibly exist, so the method cache would never be hit.

That is not the true: occasionally a new class could be placed at the same object slot. Then method cache item will be considered as cache-hit, but will point to wrong direction.

#11 Updated by Koichi Sasada about 1 year ago

  • Subject changed from Don't invalidate method caches when defining a new method on a class without subclasses to Don&#x27;t invalidate method caches when defining a new method on a class without subclasses
  • Assignee set to Koichi Sasada

#12 Updated by Charlie Somerville 4 months ago

  • Status changed from Open to Closed

Superseded by klasscache

#13 Updated by Charles Nutter 4 months ago

Is "klasscache" a reference to some other patch/issue?

Am I understanding correctly when I say that JRuby would not benefit from this sort of fix? In JRuby there is no global cache, so the only cache damage caused by a singleton class is that call sites encountering it will have to cache for the first time or fail a type test if they already cached a method. That's not great, but it's not a global invalidation event either.

#14 Updated by Charlie Somerville 4 months ago

@headius: klasscache refers to #8426/r42822 which implements JRuby style method cache invalidation.

#15 Updated by Charles Nutter 4 months ago

Righto, thanks!

Also available in: Atom PDF