Batch up all `touch` calls made in a transaction. #18824

BMorearty · 2015-02-06T07:26:59Z

Greatly reduces the number of database round-trips made when touch is called on a lot of records in a transaction. It does so by delaying the touch calls until just before the transaction is committed. At that point, it consolidates the touches into the smallest possible number of SQL UPDATEs by (a) only touching each record once, even if it was touched repeatedly in the block, and (2) touching multiple records of a single table simultaneously, using an IN clause.

after_touch calls are also deferred until right after the database updates occur, just before the commit.

The original code is from the activerecord-delay_touching gem, which has been in production on multiple sites for a bit over six months.

Some benchmarks:

I wrote a sample app where Person has_many :pets and Pet belongs_to :person, touch: true. So each time a pet is updated, the person who owns it is touched. I made a person who has 25 pets. Then I ran this code in Rails 5 with and without this change:
```
Benchmark.ips do |x| 
  ids = person.pet_ids
  x.report do
    h = ids.map { |id| { id:id, name:rand } }
    # Update all 25 pets. Touches person 25x without the change, 1x with it.
    person.update pets_attributes: h 
  end
end
```
The result was 52 iterations per second without this change and 91 iterations per second with it.
One of the production sites where activerecord-delay_touching has been in use is a Spree site. To benchmark this change, @mtuckergd created 6 products. He then added 75 variants to each product. He benchmarked code that processes changes to all of the products' variants and updates the master stock count for each of them. The Spree models that got saved were Variant, StockItem, and StockMovement. Three runs were with delay touching disabled, and three were with it enabled. Results:

Without delay touching (seconds) With delay touching (seconds)

21.27 10.37

20.26 10.21

21.22 10.45

Average:

20.92 10.34

That’s a 51% improvement to the average processing time.

Tests are included for edge cases such as rolling back a nested transaction, both with and without :requires_new.

Fixes #18606.

Greatly reduces the number of database round-trips made when `touch` is called on a lot of records. Fixes rails#18606.

dmitry · 2015-02-06T09:38:24Z

activerecord/test/schema/schema.rb

@@ -499,6 +499,7 @@ def except(adapter_names_to_exclude)
    t.string :name
    t.column :updated_at, :datetime
    t.column :happy_at,   :datetime
+    t.column :sad_at  ,   :datetime


Space between comma.

Whoops, I'll fix this.

arthurnn · 2015-02-06T15:30:13Z

(Please correct me if I am wrong). As far as I can tell the way you track this, is holding a reference to all records that call .touch inside a transaction. The problem about this implementation is, that GC wont collect them, so something like this would inflate memory a lot:

Post.transaction do
  100_000.times do
    Post.last.touch
  end
end

cc @tenderlove @jeremy

BMorearty · 2015-02-06T15:33:07Z

activerecord/lib/active_record/delay_touching.rb

+          end
+        end
+
+        klass.unscoped.where(klass.primary_key => records.sort).update_all(changes)


I noticed one change I forgot to make, that I have to make before this PR is merged: increment the optimistic locking column for all records if locking is enabled. (See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/persistence.rb#L478).

BMorearty · 2015-02-06T15:39:16Z

@arthurnn You are correct. I was concerned about this as well.

It should be noted that Rails already does this within a transaction for all records that have after_commit or after_rollback hooks. Any such record that is created or updated within a transaction will be held in memory until the transaction is finished.

In the case of touching, I've have a proposed fix but I think it could be implemented in a separate PR if it becomes a problem. The proposal is to set a limit to the number of records that are held in the buffer, and flush them to the DB if we reach that number of records. If necessary, it could even be an option to transaction to let callers override the default, but I don't think that is necessary.

The only issue I can think of for such a mechanism is dealing with rollbacks consistently.

* Use `class << self`. * Make private methods public, marked with :nodoc:. * Fix spaces in schema.rb.

BMorearty · 2015-02-06T16:40:28Z

Restoring a hidden comment that's still relevant: I noticed one change I forgot to make, that I have to make before this PR is merged: increment the optimistic locking column for all records if locking is enabled. (See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/persistence.rb#L478).

egilburg · 2015-02-06T17:29:17Z

activerecord/lib/active_record/delay_touching.rb

+        end
+
+        state.updated klass, attrs, records
+        records.each { |record| record._run_touch_callbacks }


records.each(&:_run_touch_callbacks)

egilburg · 2015-02-06T17:36:44Z

Thanks for your work on this.

With all the touch logic splittered across multiple places (Persistence, NoTouching, DelayTouching, etc) I wonder whether it'd be more manageable to aggregate all that code into a central Touch module? (it can then still break up implementation into the above mixins or similar, but it'd be top-down structured and encapsulated to the outside, rather than bubble up layers of complexity.

Anyone else has an opinion on this?

dmitry · 2015-02-06T17:50:11Z

I agree with @egilburg. It's a bit spread through the code. But it should be done in a separate PR.

BMorearty · 2015-02-06T18:07:47Z

That's an interesting proposal, @egilburg. It's a tricky thing, trying to figure out the right way to organize the touch code. Transaction also overrides touch.

If there ends up being agreement that the touch code needs to be reorganized, I would strongly suggest it be done in a separate PR after this one is complete. Refactoring is easier to test, review, and understand if it's done separately from changing/adding features.

Using benchmark-ips in Ruby 2.2, I found that: 1. Adding records to a SortedSet is just as fast as adding them to a Set. 2. Calling `sorted_set.to_a` and iterating over the results is 337 times faster than calling `set.sort` and iterating over those results. (42,180 vs. 125 iterations per second.) The memory usage is the same, because both methods create a temporary array.

Just as with a `touch` that occurs outside a transaction, this code will update the optimistic locking column but will not fail if the column was changed by another instance. When updating instances in batch, it's impossible to use the lock_version to determine whether another instance has modified the record.

BMorearty · 2015-02-06T20:33:57Z

I have fixed the issue I raised about incrementing the optimistic locking column. The fix is in the touch_records method.

dhh · 2015-02-07T04:08:31Z

activerecord/lib/active_record/delay_touching/state.rb

+      end
+
+      protected
+


✂️ newline and indent under the privacy.

dhh · 2015-02-07T04:14:15Z

Lovely work on this. I'll let others help with the detailed review, but this will be a great addition to Rails. I've seen more than my fair share of crazy touch cascades.

kaspth · 2015-02-07T09:01:27Z

activerecord/lib/active_record/delay_touching.rb

+
+      delegate :add_record, to: :state
+
+      # Are we currently executing in a delay_touching block?


✂️ this line

kaspth · 2015-02-11T14:01:41Z

activerecord/lib/active_record/delay_touching.rb

+      # touched records with the outer transaction's touched records.
+      def merge_transactions
+        num_states = states.length
+        states[num_states - 2].merge!(current_state) if num_states > 1


What happened to states[-2].merge!(current_state) if delay_touching? 😄

Oops, I missed the [-2]. I'll fix that.

I can't use if delay_touching? because that will return true even when there is only one state on the stack. I need to make sure there are two or more states.

BMorearty · 2015-02-11T18:06:14Z

I've applied all your recommendations except the ones I pushed back on. 🤘

jeremy · 2015-02-11T19:42:22Z

@BMorearty Consider that we may not need touch batching, but just touch coalescing, to alleviate the primary pains with #touch — and that could significantly reduce our implementation complexity.

The big pains are deadlocks and lock contention on parent records. Since #touch ordering is unpredictable and scattered throughout the transaction, we suffer deadlocks that are difficult to diagnose and eliminate. Since #touch is repeated on parent records, we suffer long exclusive locks on high-contention records, blocking concurrent transactions on the same parents.

We can nail both of these by marking records when they're touched, then performing the actual touch (the same one AR would immediately issue) before commit. Here's a sketch from a plugin form of this: https://gist.github.com/jeremy/84134ad08f2a137aa1ef

This ensures that the record gets an updated timestamp immediately, that repeated touches are coalesced into a single touch per record, and that touches are performed in a predictable order at the end of the transaction. It depends on couple of AR improvements: before_commit #18903, commit callbacks with manual record enrollment #18904, and touching with an explicit :time #18905.

These improvements equip us with a toolbox to make a straightforward implementation of touch coalescing and go on to reuse the same tools to do other kinds of activity coalescing within AR, plugins, and apps.

What do you think about starting with touch coalescing only, then expanding to batching as a second exploration that builds on that?

BMorearty · 2015-02-11T23:39:45Z

@jeremy Thanks for the feedback. Here are my thoughts.

The big pains are deadlocks and lock contention on parent records.

These were not the big pains we experienced on a Rails project last summer that prompted me to write the activerecord-delay_touching gem, on which this PR is based.

We didn't run across lock contention. For us, the big pain was the large amount of time it would take to touch a lot of records. There was a huge cascade of touches. Some of them would be fixed with your proposal. For example, when updating a lot of Variants (this was a Spree-based project), the associated Product was touched many times. Your proposal would reduce that to one touch for the Product. However, Spree has a complex model that also causes it to touch multiple objects in the same table. One example: a Product can belong to multiple Categories. When the Product is touched, all associated Categories are touched.

Anyway, that's the background on why I wrote this the way I did, not only coalescing touches on a single record but batching the touches that occur on multiple records in a table.

dhh · 2015-02-11T23:42:51Z

We could coalesce many updates to one model type at the end as well. I don't think that precludes that. When we mark "this needs updating", we could mark that as touches[Product] << id or whatever, and then the final execution of the touching is clever.

dhh · 2015-02-11T23:43:11Z

I think the main simplification is to move to a mark-and-bulk-execution-at-the-end model.

BMorearty · 2015-02-12T00:37:21Z

@jeremy Your gist is certainly simpler than the code in this PR.

Part of the reason for the complexity in the PR was to handle nested transactions as they're documented--by which I mean, a rollback from an inner :requires_new transaction will not update the timestamp on the records in the inner transaction, and a rollback from an inner non-:requires_new transaction will still touch them if the outer is committed. I'm not sure if this requirement will be met with the code in the gist.

Another, more important requirement that cause a bit of complexity was that side-effects (more touches) from after_touch will also be coalesced. It doesn't look to me like the gist addresses that. The first level of touches and after_touch hooks will execute, but if they cause more touches, I don't think those will execute.

If we eliminate requirements like those, and eliminate the ability to coalesce all touches in a single table, then yes, the code can be simpler, as the gist shows. I'm certain we don't want to eliminate the side-effects requirement, though, right? Because then belongs_to with touch: true will only bubble up one level.

(Or we could still coalesce touches in a single table, as @dhh suggested, but that would re-introduce some of the complexity in this PR. E.g., go back to keying off not only the class, but also the columns that have to be touched. And either live with the timestamps having different values in memory than they have on the DB, or update memory and the DB together, as I did.)

jeremy · 2015-02-12T02:49:09Z

@BMorearty after_touch will bubble once when #touch_now is called before commit. That also resolves some long-standing surprises with this callback, e.g. #8759.

Re. performance, the Spree benchmark definitely show the best of batching: small number of distinct models (2), large number of distinct parent/grandparent records to touch (150). Coalescing offers no savings when we aren't re-touching common parent/grandparent records. In Russian-doll hierarchies, the # of distinct touchable records is much smaller. Updating N child records touches the same parent and grandparent record N times. Coalescing reduces the Russian-doll case to 2 updates, same as batching. Possible examples in Spree are updating order line item quantities (1 order touch) or backing up a viewable's assets (1 viewable touch).

Re. transaction sync, before_commit fires when committing a non-joinable transaction — the outermost txn or an inner requires_new txn — so rollback/commit "just works" with the existing transaction state sync machinery.

BMorearty · 2015-02-12T04:46:53Z

after_touch will bubble once when #touch_now is called before commit.

Once isn't enough, right? We'd need to bubble it all the way up.

I think my use of the word "batch" in the title of this PR may have caused some confusion about its purpose. I just want to clarify that the primary purpose of this code--the reason I wrote it--is the same as the primary purpose you described: the touch: true case that causes one record to be touched over and over. The fact that my code also touches multiple records in a single round-trip is secondary, for the Spree case.

jeremy · 2015-02-12T14:27:27Z

Right, I mean "once" in the "when" sense, ha!

We're on the same page re. purpose 😁

On Wednesday, February 11, 2015, Brian Morearty notifications@github.com
wrote:

after_touch will bubble once when #touch_now is called before commit.

Once isn't enough, right? We'd need to bubble it all the way up.

I think my use of the word "batch" in the title of this PR may have caused
some confusion about its purpose. I just want to clarify that the primary
purpose of this code--the reason I wrote it--is the same as the primary
purpose you described: the touch: true case that causes one record to be
touched over and over. The fact that my code also touches multiple records
in a single round-trip is secondary, for the Spree case.

—
Reply to this email directly or view it on GitHub
#18824 (comment).

dhh · 2015-02-20T17:20:01Z

What's the next step for this? @BMorearty is it clear how to move forward with the before_commit approach?

BMorearty · 2015-02-20T17:58:58Z

Yes, @dhh, I think it's clear, but it sounds like this approach is completely different from the approach in the gem I wrote. I originally volunteered because I had already written the code and it had been working in production for a while (and it was an improvement over the current behavior of touches). If someone else wants to run with this new approach, I'm totally fine with that.

By the way, I'm still not convinced the before_commit approach--as written in the gist--will correctly bubble up all side-effect touches to the Nth degree. It would need to be tested for that.

dhh · 2015-02-20T18:01:25Z

I understand, Brian. Thanks for your work on this so far. Let’s open the issue up again to other contributors to take a swing at the before_commit approach. If that doesn’t pan out, I’m sure we’ll be back on this train ;)

On Feb 20, 2015, at 09:59, Brian Morearty notifications@github.com wrote:

Yes, @dhh, I think it's clear, but it sounds like this approach is completely different from the approach in the gem I wrote. I originally volunteered because I had already written the code and it had been working in production for a while (and it was an improvement over the current behavior of touches). If someone else wants to run with this new approach, I'm totally fine with that.

By the way, I'm still not convinced the before_commit approach--as written in the gist--will correctly bubble up all side-effect touches to the Nth degree. It would need to be tested for that.

—
Reply to this email directly or view it on GitHub.

BMorearty · 2015-02-20T18:08:04Z

👍

dhh · 2015-02-20T18:12:20Z

@arthurnn is going to give it a go on the before_commit approach. We'll reconvene in a new PR when it's ready.

kaspth · 2015-02-20T19:41:12Z

@BMorearty thanks, Brian. I hope you enjoyed the reviews 😄

Batch up all touch calls made in a transaction.

4658253

Greatly reduces the number of database round-trips made when `touch` is called on a lot of records. Fixes rails#18606.

BMorearty mentioned this pull request Feb 6, 2015

Limit touching to once per transaction #18606

Closed

dmitry reviewed Feb 6, 2015
View reviewed changes

carlosantoniodasilva added the activerecord label Feb 6, 2015

carlosantoniodasilva added this to the 5.0.0 milestone Feb 6, 2015

BMorearty reviewed Feb 6, 2015
View reviewed changes

Changes based on code review.

c072508

* Use `class << self`. * Make private methods public, marked with :nodoc:. * Fix spaces in schema.rb.

egilburg reviewed Feb 6, 2015
View reviewed changes

Update based on code review.

1b5f7e5

BMorearty added 3 commits February 6, 2015 10:20

Treat touch :column and touch "column" as identical.

bb795e1

dhh reviewed Feb 7, 2015
View reviewed changes

kaspth reviewed Feb 7, 2015
View reviewed changes

kaspth reviewed Feb 11, 2015
View reviewed changes

BMorearty added 2 commits February 11, 2015 10:03

Changes based on code review.

b586c19

Consistently use the term 'columns', not 'attrs'.

172fdec

andyatkinson mentioned this pull request Feb 13, 2015

:time option for #touch #18905

Closed

arthurnn mentioned this pull request Feb 14, 2015

Spike on new transaction callbacks #18936

Merged

dhh closed this Feb 20, 2015

rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

splattael mentioned this pull request Feb 17, 2022

Make before_commit hook public API #44458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch up all `touch` calls made in a transaction. #18824

Batch up all `touch` calls made in a transaction. #18824

BMorearty commented Feb 6, 2015

dmitry Feb 6, 2015

BMorearty Feb 6, 2015

arthurnn commented Feb 6, 2015

BMorearty Feb 6, 2015

BMorearty commented Feb 6, 2015

BMorearty commented Feb 6, 2015

egilburg Feb 6, 2015

egilburg commented Feb 6, 2015

dmitry commented Feb 6, 2015

BMorearty commented Feb 6, 2015

BMorearty commented Feb 6, 2015

dhh Feb 7, 2015

BMorearty Feb 8, 2015

dhh commented Feb 7, 2015

kaspth Feb 7, 2015

BMorearty Feb 8, 2015

kaspth Feb 11, 2015

BMorearty Feb 11, 2015

kaspth Feb 11, 2015

BMorearty commented Feb 11, 2015

jeremy commented Feb 11, 2015

BMorearty commented Feb 11, 2015

dhh commented Feb 11, 2015

dhh commented Feb 11, 2015

BMorearty commented Feb 12, 2015

jeremy commented Feb 12, 2015

BMorearty commented Feb 12, 2015

jeremy commented Feb 12, 2015

dhh commented Feb 20, 2015

BMorearty commented Feb 20, 2015

dhh commented Feb 20, 2015

BMorearty commented Feb 20, 2015

dhh commented Feb 20, 2015

kaspth commented Feb 20, 2015


		delegate :add_record, to: :state

		# Are we currently executing in a delay_touching block?

Without delay touching (seconds)	With delay touching (seconds)
21.27	10.37
20.26	10.21
21.22	10.45
Average:
20.92	10.34

Batch up all touch calls made in a transaction. #18824

Batch up all touch calls made in a transaction. #18824

Conversation

BMorearty commented Feb 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurnn commented Feb 6, 2015

Choose a reason for hiding this comment

BMorearty commented Feb 6, 2015

BMorearty commented Feb 6, 2015

Choose a reason for hiding this comment

egilburg commented Feb 6, 2015

dmitry commented Feb 6, 2015

BMorearty commented Feb 6, 2015

BMorearty commented Feb 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhh commented Feb 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BMorearty commented Feb 11, 2015

jeremy commented Feb 11, 2015

BMorearty commented Feb 11, 2015

dhh commented Feb 11, 2015

dhh commented Feb 11, 2015

BMorearty commented Feb 12, 2015

jeremy commented Feb 12, 2015

BMorearty commented Feb 12, 2015

jeremy commented Feb 12, 2015

dhh commented Feb 20, 2015

BMorearty commented Feb 20, 2015

dhh commented Feb 20, 2015

BMorearty commented Feb 20, 2015

dhh commented Feb 20, 2015

kaspth commented Feb 20, 2015

Batch up all `touch` calls made in a transaction. #18824

Batch up all `touch` calls made in a transaction. #18824