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

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

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
@BMorearty
Contributor

BMorearty commented Feb 6, 2015

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:

  1. 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.

  2. 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.

Batch up all `touch` calls made in a transaction.
Greatly reduces the number of database round-trips made when `touch` is
called on a lot of records.

Fixes #18606.
@@ -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

This comment has been minimized.

@dmitry

dmitry Feb 6, 2015

Contributor

Space between comma.

This comment has been minimized.

@BMorearty

BMorearty Feb 6, 2015

Contributor

Whoops, I'll fix this.


def add_record(record, columns)
# Include the standard updated_at column and any additional specified columns
updated_at_attrs = record.send(:timestamp_attributes_for_update_in_model)

This comment has been minimized.

@dmitry

dmitry Feb 6, 2015

Contributor

Access to the private method. Must use some kind of the getter.

This comment has been minimized.

@BMorearty

BMorearty Feb 6, 2015

Contributor

Good catch. I can change timestamp_attributes_for_update_in_model to a public method with :nodoc:. That way it will be callable from outside the record, but not documented as a part of the public ActiveRecord API.

states.last
end

class << self

This comment has been minimized.

@dmitry

dmitry Feb 6, 2015

Contributor

Why not using class << self everywhere, as in NoTouching module?

This comment has been minimized.

@BMorearty

BMorearty Feb 6, 2015

Contributor

That was an oversight on my part. I wrote the original code while on a team where the preferred style was self. and I only used class << self in places where it's required, such as the delegate declaration.

I'll fix the style to use the Rails convention.

# Touch the specified records--non-empty set of instances of the same class.
def self.touch_records(klass, attrs, records)
if attrs.present?
current_time = records.first.send(:current_time_from_proper_timezone)

This comment has been minimized.

@dmitry

dmitry Feb 6, 2015

Contributor

Second time access to the same private method.

This comment has been minimized.

@BMorearty

BMorearty Feb 6, 2015

Contributor

Actually it's a different method this time, but your point is valid. 😄 I'll make it public and mark it with :nodoc:.

@arthurnn

This comment has been minimized.

Member

arthurnn commented Feb 6, 2015

(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

end
end

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

This comment has been minimized.

@BMorearty

BMorearty Feb 6, 2015

Contributor

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

This comment has been minimized.

Contributor

BMorearty commented Feb 6, 2015

@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.

Changes based on code review.
* Use `class << self`.
* Make private methods public, marked with :nodoc:.
* Fix spaces in schema.rb.
@BMorearty

This comment has been minimized.

Contributor

BMorearty commented Feb 6, 2015

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).

end

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

This comment has been minimized.

@egilburg

egilburg Feb 6, 2015

Contributor

records.each(&:_run_touch_callbacks)

@egilburg

This comment has been minimized.

Contributor

egilburg commented Feb 6, 2015

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

This comment has been minimized.

Contributor

dmitry commented Feb 6, 2015

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

@BMorearty

This comment has been minimized.

Contributor

BMorearty commented Feb 6, 2015

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.

BMorearty added some commits Feb 6, 2015

Use SortedSet instead of Set for better performance.
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.
Support optimistic locking.
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

This comment has been minimized.

Contributor

BMorearty commented Feb 6, 2015

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

end

protected

This comment has been minimized.

@dhh

dhh Feb 7, 2015

Member

✂️ newline and indent under the privacy.

This comment has been minimized.

@BMorearty

BMorearty Feb 8, 2015

Contributor

Done.

@dhh

This comment has been minimized.

Member

dhh commented Feb 7, 2015

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.


delegate :add_record, to: :state

# Are we currently executing in a delay_touching block?

This comment has been minimized.

@kaspth

kaspth Feb 7, 2015

Member

✂️ this line

This comment has been minimized.

@BMorearty

BMorearty Feb 8, 2015

Contributor

Done.


# Are we currently executing in a delay_touching block?
def delay_touching?
DelayTouching.states.length > 0

This comment has been minimized.

@kaspth

kaspth Feb 7, 2015

Member

In this scope self is DelayTouching, so you can rely on implicit self. Could also just be !states.empty?.

This comment has been minimized.

@BMorearty

BMorearty Feb 8, 2015

Contributor

Good idea. Done. (Actually, I changed it to states.present?.)

This comment has been minimized.

@kaspth

kaspth Feb 8, 2015

Member

That'll do it 👍

ensure
merge_transactions unless $! && options[:requires_new]

# Decrement nesting even if +apply+ raised an error.

This comment has been minimized.

@kaspth

kaspth Feb 7, 2015

Member

Can you document why we need to decrement the nesting even if +apply+ raised an error?

This comment has been minimized.

@BMorearty

BMorearty Feb 8, 2015

Contributor

Yes. Done.

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

This comment has been minimized.

@kaspth

kaspth Feb 7, 2015

Member

Could this be states[-2].merge!(state) if delay_touching??

This comment has been minimized.

@BMorearty

BMorearty Feb 8, 2015

Contributor

Sure, that's more clear. Done.

Thread.current[:delay_touching_states] ||= []
end

def state

This comment has been minimized.

@kaspth

kaspth Feb 7, 2015

Member

I'd suggest renaming this to current_state. I keep thinking it's a local variable in some of the methods below when state is just there.

This comment has been minimized.

@BMorearty

BMorearty Feb 8, 2015

Contributor

Good idea. current_state is a better name.

end

# Apply the touches that were delayed. We're in a transaction already so there's no need to open one.
def apply

This comment has been minimized.

@kaspth

kaspth Feb 7, 2015

Member

This could be (note the rename):

# Applies delayed touches within the current transaction.
def apply_touches
  begin
    state.get_and_clear_records.each do |(klass, attrs), records|
      touch_records klass, attrs, records
    end
  end while state.more_records?
ensure
  state.clear_already_updated_records
end

Why do we have to use the begin ... while form instead of while ...?

This comment has been minimized.

@BMorearty

BMorearty Feb 8, 2015

Contributor

Beautiful refactor, @kaspth. I've made this change and moved the while to the top.

delegate :add_record, to: :current_state

def delay_touching?
states.present?

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

Come to think of it any? might be better here 😄

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

I'd rather leave this as present? if you don't mind. It's a standard part of ActiveSupport, I think it's just as readable, and it doesn't require calling a block.

# 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

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

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

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

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.

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

Right 👍

end
ensure
current_state.clear_already_touched_records
end

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

apply_touches will only be called when there's a single State on the stack. It also seems to know a lot about the current_state, so perhaps we should push this behavior into the state.

Then start could be current_state.apply_touches if states.length == 1 instead. What do you think? 😄

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

Whoops, I forgot to ask if using a while loop here is needed (as opposed to an if statement), since apply_touches is called with one state left? As far as I can tell there won't suddenly be more records after touch_records is called.

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

... so perhaps we should push this behavior into the state.

My intent was that the state doesn't do the actual touching, it just keeps track of which records need to be touched and have already been touched. It's there as a helper for the main logic, which is in the DelayTouching module. If State does the actual touching, the name "State" seems wrong. I think it's better as is.

I forgot to ask if using a while loop here is needed (as opposed to an if statement)....

An if statement won't work here. (I verified this by trying it.) The reason for the while loop is that each time an after_touch callback is called, it can have a side-effect of causing more calls to touch on other objects. (E.g., belongs_to :something, touch: true.) The while loop in apply_touches is there so that we can keep on iterating until there are no more touches caused by side-effects.

record.instance_eval do
attrs.each { |column| write_attribute column, current_time }
increment_lock if locking_enabled?
@changed_attributes.except!(*attrs)

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

@sgrif is there a better way to accomplish this without reaching into an ivar?

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

Thanks. When I first wrote this code last summer for Rails 4, I don't think clear_attribute_changes existed. Or if it did, touch wasn't calling it. I'll update it.

end

# Touch the specified records--non-empty set of instances of the same class.
def touch_records(klass, attrs, records)

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

There's an inconsistency here with attrs. Some places you say columns versus attrs in others. This confused me because I kept thinking this was an attrs hash with values instead of an array of time columns to be updated.

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

Oh yeah, good point. I'll change it to columns here and in State.

states.push State.new
result = yield
apply_touches if states.length == 1
result

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

I would probably tap here:

yield.tap do
  apply_touches if states.length == 1
end

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

I avoided tap on purpose. I felt it was less readable and takes the same number of lines (if using do/end, not {}).

This comment has been minimized.

@sgrif

sgrif Feb 11, 2015

Member

It's less about the number of lines, and more about clarifying intent. I can immediately see that the return value will be yield, and you just need to do something else before you return.

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

@sgrif If you really want tap, I'll change it. But we have a difference of opinion here. I believe tap obfuscates the intent.

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

Awaiting a further reply before proceeding--will you allow the way I think is clearer (no tap), or shall I use the way you think is clearer (tap)? 😃

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

As long as you tap into your ❤️ when programming, either way is fine 😄

This comment has been minimized.

@BMorearty

BMorearty Feb 11, 2015

Contributor

❤️

This comment has been minimized.

@stuarthannig

stuarthannig Feb 14, 2015

I feel tap is more appropriate because it consolidates the logic to a single idea, compared to having to worry about the result variable showing up twice. Same amount of lines, true, but to me I understand what I am getting out of the code with the do block. That's my input, thanks for your work though it's great regardless.

@BMorearty

This comment has been minimized.

Contributor

BMorearty commented Feb 11, 2015

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

@jeremy

This comment has been minimized.

Member

jeremy commented Feb 11, 2015

@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

This comment has been minimized.

Contributor

BMorearty commented Feb 11, 2015

@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

This comment has been minimized.

Member

dhh commented Feb 11, 2015

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

This comment has been minimized.

Member

dhh commented Feb 11, 2015

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

@BMorearty

This comment has been minimized.

Contributor

BMorearty commented Feb 12, 2015

@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

This comment has been minimized.

Member

jeremy commented Feb 12, 2015

@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

This comment has been minimized.

Contributor

BMorearty commented Feb 12, 2015

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

This comment has been minimized.

Member

jeremy commented Feb 12, 2015

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

This comment has been minimized.

Member

dhh commented Feb 20, 2015

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

@BMorearty

This comment has been minimized.

Contributor

BMorearty commented Feb 20, 2015

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

This comment has been minimized.

Member

dhh commented Feb 20, 2015

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

This comment has been minimized.

Contributor

BMorearty commented Feb 20, 2015

👍

@dhh

This comment has been minimized.

Member

dhh commented Feb 20, 2015

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

@dhh dhh closed this Feb 20, 2015

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 20, 2015

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment