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

PERF: 2x ~ 30x faster dirty tracking #35933

Merged
merged 1 commit into from Apr 11, 2019
Merged

Conversation

@kamipo
Copy link
Member

@kamipo kamipo commented Apr 11, 2019

Currently, although using both dirty tracking (ivar backed and
attributes backed) on one model is not supported (doesn't fully work at
least), both dirty tracking are being performed, that is very slow.

As long as attributes backed dirty tracking is used, ivar backed dirty
tracking should not need to be performed.

I've refactored to extract new ForcedMutationTracker which only tracks
force_change to be performed for ivar backed dirty tracking, that
makes dirty tracking on Active Record 2x ~ 30x faster.

https://gist.github.com/kamipo/971dfe0891f0fe1ec7db8ab31f016435

Before:

Warming up --------------------------------------
            changed?     4.467k i/100ms
             changed     5.134k i/100ms
             changes     3.023k i/100ms
  changed_attributes     4.358k i/100ms
        title_change     3.185k i/100ms
           title_was     3.381k i/100ms
Calculating -------------------------------------
            changed?     42.197k (±28.5%) i/s -    187.614k in   5.050446s
             changed     50.481k (±16.0%) i/s -    246.432k in   5.045759s
             changes     30.799k (± 7.2%) i/s -    154.173k in   5.030765s
  changed_attributes     51.530k (±14.2%) i/s -    252.764k in   5.041106s
        title_change     44.667k (± 9.0%) i/s -    222.950k in   5.040646s
           title_was     44.635k (±16.6%) i/s -    216.384k in   5.051098s

After:

Warming up --------------------------------------
            changed?    24.130k i/100ms
             changed    13.503k i/100ms
             changes     6.511k i/100ms
  changed_attributes     9.226k i/100ms
        title_change    48.221k i/100ms
           title_was    96.060k i/100ms
Calculating -------------------------------------
            changed?    245.478k (±16.1%) i/s -      1.182M in   5.015837s
             changed    157.641k (± 4.9%) i/s -    796.677k in   5.066734s
             changes     70.633k (± 5.7%) i/s -    358.105k in   5.086553s
  changed_attributes     95.155k (±13.6%) i/s -    470.526k in   5.082841s
        title_change    566.481k (± 3.5%) i/s -      2.845M in   5.028852s
           title_was      1.487M (± 3.9%) i/s -      7.493M in   5.046774s
Currently, although using both dirty tracking (ivar backed and
attributes backed) on one model is not supported (doesn't fully work at
least), both dirty tracking are being performed, that is very slow.

As long as attributes backed dirty tracking is used, ivar backed dirty
tracking should not need to be performed.

I've refactored to extract new `ForcedMutationTracker` which only tracks
`force_change` to be performed for ivar backed dirty tracking, that
makes dirty tracking on Active Record 2x ~ 30x faster.

https://gist.github.com/kamipo/971dfe0891f0fe1ec7db8ab31f016435

Before:

```
Warming up --------------------------------------
            changed?     4.467k i/100ms
             changed     5.134k i/100ms
             changes     3.023k i/100ms
  changed_attributes     4.358k i/100ms
        title_change     3.185k i/100ms
           title_was     3.381k i/100ms
Calculating -------------------------------------
            changed?     42.197k (±28.5%) i/s -    187.614k in   5.050446s
             changed     50.481k (±16.0%) i/s -    246.432k in   5.045759s
             changes     30.799k (± 7.2%) i/s -    154.173k in   5.030765s
  changed_attributes     51.530k (±14.2%) i/s -    252.764k in   5.041106s
        title_change     44.667k (± 9.0%) i/s -    222.950k in   5.040646s
           title_was     44.635k (±16.6%) i/s -    216.384k in   5.051098s
```

After:

```
Warming up --------------------------------------
            changed?    24.130k i/100ms
             changed    13.503k i/100ms
             changes     6.511k i/100ms
  changed_attributes     9.226k i/100ms
        title_change    48.221k i/100ms
           title_was    96.060k i/100ms
Calculating -------------------------------------
            changed?    245.478k (±16.1%) i/s -      1.182M in   5.015837s
             changed    157.641k (± 4.9%) i/s -    796.677k in   5.066734s
             changes     70.633k (± 5.7%) i/s -    358.105k in   5.086553s
  changed_attributes     95.155k (±13.6%) i/s -    470.526k in   5.082841s
        title_change    566.481k (± 3.5%) i/s -      2.845M in   5.028852s
           title_was      1.487M (± 3.9%) i/s -      7.493M in   5.046774s
```
@kamipo kamipo force-pushed the refactor_dirty_tracking branch from e830c82 to 6b0a9de Apr 11, 2019
jeremy
jeremy approved these changes Apr 11, 2019
Copy link
Member

@jeremy jeremy left a comment

Fantastic PR! Great perf boost, and much clearer code. 👏🏼

@kamipo kamipo merged commit c820d8d into rails:master Apr 11, 2019
2 checks passed
@kamipo kamipo deleted the refactor_dirty_tracking branch Apr 11, 2019
@shioyama
Copy link
Contributor

@shioyama shioyama commented Apr 21, 2019

Performance boost is absolutely fantastic, but wondering about this:

Currently, although using both dirty tracking (ivar backed and
attributes backed) on one model is not supported (doesn't fully work at
least)

Is this something that would be welcomed as a PR, if it had no impact on performance (i.e. if it could maintain the perf here)? I've hacked around this area in Mobility for a while and every change to ActiveRecord requires a new hack.

I personally think it's really a shame that the combination of ivar-backed dirty attributes and attribute-backed attributes is not supported on AR models. It means that the kind of thing users want from my gem is very hard to support without ugly monkey-patching.

@kamipo
Copy link
Member Author

@kamipo kamipo commented Apr 22, 2019

I've read the code https://github.com/shioyama/mobility/blob/master/lib/mobility/plugins/active_record/dirty.rb and https://github.com/shioyama/mobility/blob/master/lib/mobility/plugins/active_model/dirty.rb,
it already deeply hackes/depends on internal API and internal state (e.g. model.send(:attributes_changed_by_setter).except!(locale_accessor) should be model.clear_attribute_changes(*locale_accessor), hacking force tracking both (ivar/attributes) previous changes, etc.).

I'm not prefer to guarantee such dirty hacks.

@shioyama
Copy link
Contributor

@shioyama shioyama commented Apr 22, 2019

I'm not prefer to guarantee such dirty hacks.

I completely agree! And I don't expect Rails to guarantee dirty hacks. I tried to find a public API that would allow using simple ActiveModel dirty tracking on an ActiveRecord model, but there is none AFAICT. AR overrides AM methods so only AR dirty tracking works. You can't use them independently.

I don't want dirty hacks, I just want to be able to say: this attribute is virtual, not persisted, and track changes on it, without having to declare it's type with attribute. That's the API which I need.

I think Rails should be more modular so you can include these kinds of things without one overriding the other. Internally, it's also (honestly) quite ugly how AR overrides private methods of AM in many places.

I don't know how to do it (yet), but I'd like to at least have a look at whether it is possible. That was my motivation for #31394, which I closed (I don't think that PR is the correct solution now).

@shioyama
Copy link
Contributor

@shioyama shioyama commented Apr 22, 2019

To be clear, #31394 would have allowed this by exposing a module builder ActiveModel::AttributeMethodsBuilder whose instances could have been used alongside instances of ActiveRecord::AttributeMethodsBuilder in the model's ancestors. It also encapsulated internals of the dirty modules into the module builder so they were not exposed in the model class itself.

@shioyama
Copy link
Contributor

@shioyama shioyama commented Apr 22, 2019

e.g. model.send(:attributes_changed_by_setter).except!(locale_accessor) should be model.clear_attribute_changes(*locale_accessor), hacking force tracking both (ivar/attributes) previous changes, etc.).

I'm pretty sure I tried this and it did not work (at the time anyway). I have looked quite carefully at the AR code and tried hard to make the hack as minimal as possible. It is very tricky...

jrafanie added a commit to jrafanie/ancestry that referenced this issue May 21, 2019
Fixes a deprecation warning if an after callback tries to call
ancestry_changed?

Note, the !! is required because rails 5.1 and 5.2 return nil in these
methods if they have no changes.  This was fixed in a refactoring in rails 6.0:
rails/rails#35933
@jrafanie
Copy link
Contributor

@jrafanie jrafanie commented May 21, 2019

Note, this PR indirectly "fixed" the predicate methods will_save_change_to_attribute? and saved_change_to_attribute? (and probably others) returning nil instead of false when there are no changes to the attribute. It's a small change that eliminates the need for me to use !! in my code. Thanks @kamipo !!

@welearnednothing
Copy link

@welearnednothing welearnednothing commented May 27, 2019

I'm trying to use Neo4j with 6.0.0.rc1 but when I try to do a migration (rails neo4j:migrate) I get an error NoMethodError: undefined method force_change' for #ActiveModel::NullMutationTracker:0x00007fbed543e338`. It looks like I'm not alone.

This commit removed the methods forget_change, original_value, and force_change from ActiveModel::NullMutationTracker, making it no longer API compatible with ActiveModel::AttributeMutationTracker or the new ActiveModel::ForcedMutationTracker. These three methods were no-ops, so it may have seemed as though they were no longer necessary, but now use cases that relied on NullMutationTracker and called those methods will fail.

Was there a motivation for deleting these methods, or were they seen as not needed since they appeared to do nothing? I'm happy to put together a pull request putting them back in place if this was an oversight or push up an example repo of the failure if it's helpful.

@kamipo
Copy link
Member Author

@kamipo kamipo commented May 28, 2019

The motivation that removed forget_change and force_change in the NullMutationTracker is to disallow any mutation to the mutations_before_last_save mutation tracker.

If Neo4j will mutate mutations_before_last_save or use NullMutationTracker internal class directly, it is already something wrong.

Seems it should be addressed in the Neo4j side to me.

@kamipo
Copy link
Member Author

@kamipo kamipo commented May 28, 2019

Perhaps reverting the commit neo4jrb/activegraph@3b1d1bf will avoid the NoMethodError.

jrafanie added a commit to jrafanie/ancestry that referenced this issue May 29, 2019
Fixes a deprecation warning if an after callback tries to call
ancestry_changed?

Note, the !! is required because rails 5.1 and 5.2 return nil in these
methods if they have no changes.  This was fixed in a refactoring in rails 6.0:
rails/rails#35933
@welearnednothing
Copy link

@welearnednothing welearnednothing commented Jun 4, 2019

@kamipo Thank you so much for the follow up and pointing me to that Neo4j commit - I'm following up with the author of that commit. Are you saying NullMutationTracker shouldn't be used directly outside of Rails?

@kamipo
Copy link
Member Author

@kamipo kamipo commented Jun 4, 2019

Are you saying NullMutationTracker shouldn't be used directly outside of Rails?

Yes, that is an internal class # :nodoc:ed explicitly.

@welearnednothing
Copy link

@welearnednothing welearnednothing commented Jun 4, 2019

Got it, I'll work on getting this fixed in the Neo4j gem. Thanks for your help, @kamipo!

@Joshfindit
Copy link

@Joshfindit Joshfindit commented Aug 21, 2019

Which module needs to be required in order to support the force_change method?
Using Sinatra.

Edit: presumably require 'active_model/attribute_mutation_tracker', but activemodel-6.0.0/lib/active_model/attribute_mutation_tracker.rb does not contain the force_change function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants