Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Committing large transactions is very slow #48

Merged
merged 1 commit into from
Mar 23, 2013
Merged

Committing large transactions is very slow #48

merged 1 commit into from
Mar 23, 2013

Conversation

jturkel
Copy link
Contributor

@jturkel jturkel commented Mar 22, 2013

Committing transactions involving large numbers of hierarchy model classes is very slow. On my machine committing a transaction involving 1,111 hierarchy model classes takes 5.2 secs.

The problem appears to be that the hierarchy model class overrides == and eql? but not hash. Thus all model instances hash to the same value making the uniq call on the array of modified ActiveRecord instances in ActiveRecord::ConnectionAdapters::DatabaseStatements.commit_transaction_records() very slow. For my test case with 1,111 hierarchy model instances, the call to uniq() makes 2,466,420 calls to the hierarchy model eql? method. If I implement the hierarchy model hash method as follows this drops to 4,326 calls and the commit completes in 0.06 secs:

def hash
  attributes.hash
end

@mceachen
Copy link
Collaborator

Great sleuthing, thanks!

I'd like a new test to make sure the issue doesn't come back later, but that shouldn't be hard.

@mceachen
Copy link
Collaborator

Darn, looks like it breaks under 1.8.7. Your impl and tests look legit, though, looking into it.

@mceachen
Copy link
Collaborator

Looks like a better hash implementation is all we needed to make 1.8.7 happy. Pulled your branch into issue_48 and awaiting the build status.

@mceachen mceachen merged commit 29ec3db into ClosureTree:master Mar 23, 2013
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants