search

state-machines / state_machines

Adds support for creating state machines for attributes on any Ruby class
https://github.com/state-machines/state_machines
MIT License
814 stars 91 forks source link

Optimize Integrations::Base::ClassMethods#matches? #51

Closed casperisfine closed 7 years ago

casperisfine commented 7 years ago

Followup to: https://github.com/state-machines/state_machines/pull/50

I was investigating into our app boot time today, and I found something interesting:

==================================
  Mode: wall(1000)
  Samples: 22 (21.43% miss rate)
  GC: 2 (9.09%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
         6  (27.3%)           5  (22.7%)     StateMachines::Machine#owner_class_ancestor_has_method?
         5  (22.7%)           5  (22.7%)     block in StateMachines::Integrations::Base::ClassMethods#matches?
         1   (4.5%)           1   (4.5%)     block in ActiveSupport::Dependencies#search_for_file
         1   (4.5%)           1   (4.5%)     Hash#assert_valid_keys
         1   (4.5%)           1   (4.5%)     StateMachines::NodeCollection#value
         1   (4.5%)           1   (4.5%)     StateMachines::Machine#define_helper
         1   (4.5%)           1   (4.5%)     ActiveRecord::DynamicMatchers#respond_to?
         1   (4.5%)           1   (4.5%)     block in StateMachines::Machine#owner_class_ancestor_has_method?
         1   (4.5%)           1   (4.5%)     ActiveSupport::Callbacks::ClassMethods#get_callbacks
         1   (4.5%)           1   (4.5%)     ActiveSupport::Concern#append_features

Turns out looking out for ancestors by name is very slow. If we instead use Module#<= it basically disapears from the profile.

One problem though, this is a breaking change. All integrations would have to be updated.

One possiblity would be to constantize those names and issue a warnings, but I'm not sure if it's worth it.

@seuros @rafaelfranca thoughts?

rafaelfranca commented 7 years ago

At first look I'm fine with this change, but I want to understand why we were using string in the first place. Maybe there is something related with autoloading. This gem is not with stable API yet, so a breaking change like this is fine as soon we properly document in the changelog.

casperisfine commented 7 years ago

Maybe there is something related with autoloading.

I supposed so as well. But I don't think it is. If you look at the integrations the load order isn't a problem at all.

The class name is inside a method: https://github.com/state-machines/state_machines-mongoid/blob/c1a7493b545b6037c0b7e64231bda7cd30c50943/lib/state_machines/integrations/mongoid.rb#L297 so it's resolved quite a bit later

Then the integrations explicitly requires the constants they reference: https://github.com/state-machines/state_machines-mongoid/blob/c1a7493b545b6037c0b7e64231bda7cd30c50943/lib/state_machines/integrations/mongoid.rb#L3

So I really don't see why it would need to use strings.

I also tested a patched Active Record integration locally, and it worked like a charm.

jhawthorn commented 7 years ago

@casperisfine @rafaelfranca is it possible to send those patches to the various integrations? They are all broken with the new 0.5.0 release. (state_machines-activemodel, state_machines-activerecord, state_machines-activemodel-observers, state_machines-mongoid)

rafaelfranca commented 7 years ago

Yeah, of course. I'm working on them

rafaelfranca commented 7 years ago

I update state_machines-activemodel, state_machines-activerecord and state_machines-activemodel-observers. I'm waiting access to the others to be able to release

rafaelfranca commented 7 years ago

They should be all fixed now

jhawthorn commented 7 years ago

So quick. Thanks so much. :heart:

seuros commented 7 years ago

Thank you @casperisfine for this PR. And thanks to @rafaelfranca for the release.