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

Avoid method call if `@transaction_state` is not finalized #36049

Merged
merged 1 commit into from Apr 21, 2019

Conversation

Projects
None yet
6 participants
@kamipo
Copy link
Member

commented Apr 21, 2019

Method call in Ruby is a bit slow.

This makes attribute access 10% faster by avoiding method call
(sync_with_transaction_state).

Before (96cf7e0):

Warming up --------------------------------------
             user.id   131.291k i/100ms
          user['id']    91.786k i/100ms
           user.name   151.605k i/100ms
        user['name']    92.664k i/100ms
       user.changed?    17.772k i/100ms
 user.saved_changes?    23.909k i/100ms
Calculating -------------------------------------
             user.id      1.988M (± 7.0%) i/s -      9.978M in   5.051474s
          user['id']      1.155M (± 5.8%) i/s -      5.783M in   5.022672s
           user.name      2.450M (± 4.3%) i/s -     12.280M in   5.021234s
        user['name']      1.263M (± 2.1%) i/s -      6.394M in   5.066638s
       user.changed?    175.070k (±13.3%) i/s -    853.056k in   5.011555s
 user.saved_changes?    259.114k (±11.8%) i/s -      1.267M in   5.001260s

After (this change):

Warming up --------------------------------------
             user.id   137.625k i/100ms
          user['id']    96.054k i/100ms
           user.name   156.379k i/100ms
        user['name']    94.795k i/100ms
       user.changed?    18.172k i/100ms
 user.saved_changes?    24.337k i/100ms
Calculating -------------------------------------
             user.id      2.201M (± 0.5%) i/s -     11.010M in   5.002955s
          user['id']      1.320M (± 1.0%) i/s -      6.628M in   5.021293s
           user.name      2.677M (± 1.6%) i/s -     13.449M in   5.024399s
        user['name']      1.314M (± 1.8%) i/s -      6.636M in   5.051444s
       user.changed?    190.588k (±11.1%) i/s -    944.944k in   5.065848s
 user.saved_changes?    262.782k (±12.1%) i/s -      1.290M in   5.028080s

cc @shioyama

Avoid method call if `@transaction_state` is not finalized
Method call in Ruby is a bit slow.

This makes attribute access 10% faster by avoiding method call
(`sync_with_transaction_state`).

Before (96cf7e0):

```
Warming up --------------------------------------
             user.id   131.291k i/100ms
          user['id']    91.786k i/100ms
           user.name   151.605k i/100ms
        user['name']    92.664k i/100ms
       user.changed?    17.772k i/100ms
 user.saved_changes?    23.909k i/100ms
Calculating -------------------------------------
             user.id      1.988M (± 7.0%) i/s -      9.978M in   5.051474s
          user['id']      1.155M (± 5.8%) i/s -      5.783M in   5.022672s
           user.name      2.450M (± 4.3%) i/s -     12.280M in   5.021234s
        user['name']      1.263M (± 2.1%) i/s -      6.394M in   5.066638s
       user.changed?    175.070k (±13.3%) i/s -    853.056k in   5.011555s
 user.saved_changes?    259.114k (±11.8%) i/s -      1.267M in   5.001260s
```

After (this change):

```
Warming up --------------------------------------
             user.id   137.625k i/100ms
          user['id']    96.054k i/100ms
           user.name   156.379k i/100ms
        user['name']    94.795k i/100ms
       user.changed?    18.172k i/100ms
 user.saved_changes?    24.337k i/100ms
Calculating -------------------------------------
             user.id      2.201M (± 0.5%) i/s -     11.010M in   5.002955s
          user['id']      1.320M (± 1.0%) i/s -      6.628M in   5.021293s
           user.name      2.677M (± 1.6%) i/s -     13.449M in   5.024399s
        user['name']      1.314M (± 1.8%) i/s -      6.636M in   5.051444s
       user.changed?    190.588k (±11.1%) i/s -    944.944k in   5.065848s
 user.saved_changes?    262.782k (±12.1%) i/s -      1.290M in   5.028080s
```

@rails-bot rails-bot bot added the activerecord label Apr 21, 2019

@shioyama

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

Wow great! user.id also faster, that's great. Fantastic! Thanks for responding so quickly.

Just so the PRs are linked: this is in response to #35987 comment.

@shioyama

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

Just to confirm: I also see a performance bump of about 6-8% or so.

@kamipo kamipo merged commit 56ffb26 into rails:master Apr 21, 2019

2 checks passed

buildkite/rails Build #60650 passed (9 minutes, 36 seconds)
Details
codeclimate All good!
Details

@kamipo kamipo deleted the kamipo:avoid_method_call branch Apr 21, 2019

@kaspth

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

What would the perf hit be if we abstracted sync_with_transaction_state if @transaction_state&.finalized? to something like sync_with_finalized_transaction_state?

@shioyama

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

@kaspth I think the point is that any method call takes a performance hit, so checking an instance variable first (which in 99% of cases is falsey) saves that hit and improves performance.

It's not pretty though, so maybe in the future someone could come up with a cleaner solution with the same performance.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

The code is now harder to understand and maintain. It now open the possibility to someone to introduce a call to sync_with_transaction_state that doesn't check by the @transaction_state as if this was a valid call. It is not.

I totally understand that this is faster, but it is only faster because the MRI is slow. I don't think we should be spreading conditionals and avoiding method calls, breaking encapsulation, increasing maintainability cost, to compensate the limitations of the Ruby VM. If the MRI is slow we improve it, not ask everyone to change their code because we can't make MRI faster.

@shioyama

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

#36052 is similar. ivar access over method calls gives a pretty sizeable boost on getters.

Maybe the change could be limited to only _read_attribute (with a comment explaining why it's there), since that's the one that originally incurred the 10% hit to user.name? The others probably make less of a difference (%-wise). Then just revert the change to sync_with_transaction_state so it also checks finalized? (so _read_attribute would check twice in case it's true, but it's almost always false).

@SamSaffron

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@rafaelfranca @kamipo @k0kubun I wonder if we can rely longer term on methods being inlined / erased with:

sync_with_transaction_state 

def sync_with_transaction_state
    sync_with_transaction_state_impl if @transaction_state&.finalized?
end

def sync_with_transaction_state_impl
   .....
end

Or even deeper magic...I totally hear @rafaelfranca's concern about risk of regressions due to code being spread out.

But also feel that the 10% figure is a pretty material win here we should not be giving up.

@kamipo

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Note that sync_with_transaction_state without @transaction_state&.finalized? is still valid call.
This guard condition is just for early return, if sync_with_transaction_state is called when @transaction_state isn't finalized, all fully_committed?, committed?, fully_rolledback?, rolledback? states are falsy.

def sync_with_transaction_state
if transaction_state = @transaction_state
if transaction_state.fully_committed?
force_clear_transaction_record_state
elsif transaction_state.committed?
clear_transaction_record_state
elsif transaction_state.rolledback?
force_restore_state = transaction_state.fully_rolledback?
restore_transaction_record_state(force_restore_state)
clear_transaction_record_state
end
end
end

I agree that it is not pretty. One possible solution making code simple is reverting the GC friendly transaction #9068.
Reverting #9068 completely removes all sync_with_transaction_state in our code base.

@SamSaffron

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

We need to re-validate if we still need #9068 in current rubies? Do we have a gist demonstrating the pathological cases where it is even needed?

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

But also feel that the 10% figure is a pretty material win here we should not be giving up.

Yes, this is why I didn't reverted this change. The code is now less clear but 10% is a lot, I commented more because I don't want this trend to continue to shave off 1% in micro benchmarks. If we have more low-hanging fruits that yields gains similar to this one I'm fine on applying the same technique.

@kaspth

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@kamipo I'd be curious to see a version that reverted #9068 and what the implications of removing it might be. I remember Jon Wang worked on running multiple Rails applications in a single process back for GSoC 2013, so this could be related to that.

@@ -479,7 +479,7 @@ def has_transactional_callbacks?
# the TransactionState, and rolls back or commits the Active Record object
# as appropriate.
def sync_with_transaction_state
if (transaction_state = @transaction_state)&.finalized?

This comment has been minimized.

Copy link
@gklsan

gklsan Apr 29, 2019

Why not write

Suggested change
if (transaction_state = @transaction_state)&.finalized?
return unless @transaction_state&.finalized?
if transaction_state = @transaction_state

If we use this no need to touch the remaining 13 places right?

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 29, 2019

Member

Because that’s exactly the same as the previous code and thus wouldn’t give any performance boost. The boost comes from the transaction state being false for the majority of calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.