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

4.2 perf: AR new with serialized attrs up to 10x slower #18029

Closed
mfazekas opened this issue Dec 14, 2014 · 9 comments
Closed

4.2 perf: AR new with serialized attrs up to 10x slower #18029

mfazekas opened this issue Dec 14, 2014 · 9 comments
Assignees
Milestone

Comments

@mfazekas
Copy link
Contributor

We're seeing about +30% time increase in execution time, when running our rspec. (this is similiar to what reported at the end of #17472)
Profiling showed much more YAML parse(psych) compared to 4.1.

Given:

class DummyItem < ActiveRecord::Base
  serialize :data1, Array
  serialize :data2, Array
  serialize :data3, Array
  serialize :data4, Array

  after_initialize :init

  def init
    self.num = 42
  end
end

puts Benchmark.measure { 3000.times { DummyItem.new } }

is about 0.2 s in rails 4.1 vs 2.2 s in rails 4.2.rc3.

I don't think it affects web app performance much, but since we use factories and not fixtures for testing, the test execution speed suffers.

full testcase: https://gist.github.com/mfazekas/897f857e4367c94b771b

@mfazekas
Copy link
Contributor Author

I have a quick WIP optimization: mfazekas@1800232
this seems to reduce time to about 0.5 s which is "only" 2.5x slower than rails 4.1 an 4x improvement from 4.2.rc3 for this usecase. This improves our rspec suite by about 10%.

@mfazekas mfazekas changed the title 4.2 perf AR new with serialized attrs up to 10x slower 4.2 perf: AR new with serialized attrs up to 10x slower Dec 14, 2014
@nateberkopec
Copy link
Contributor

Just confirming using IPS instead:

# Rails 4.1.8
Calculating -------------------------------------
                         1.162k i/100ms
-------------------------------------------------
                         13.180k (± 5.3%) i/s -     66.234k
# Rails 4.2.0rc3
Calculating -------------------------------------
                       106.000  i/100ms
-------------------------------------------------
                          1.122k (± 9.1%) i/s -      5.618k

@nateberkopec
Copy link
Contributor

Worth noting that the patch above doesn't do much for you if the serialized attr isn't nil. Modified test script here.

@chancancode chancancode added this to the 4.2.1 milestone Dec 15, 2014
sgrif added a commit that referenced this issue Dec 22, 2014
Calling `changed_attributes` will ultimately check if every mutable
attribute has changed in place. Since this gets called whenever an
attribute is assigned, it's extremely slow. Instead, we can avoid this
calculation until we actually need it.

Fixes #18029
@sgrif
Copy link
Contributor

sgrif commented Dec 22, 2014

Thanks for the report. The fix will be included in 4.2.1

@sgrif sgrif closed this as completed in 18ae065 Dec 22, 2014
@nateberkopec
Copy link
Contributor

Thanks @sgrif!

@sgrif
Copy link
Contributor

sgrif commented Dec 22, 2014

Also, using the benchmark provided, I'm seeing a significant speed-up on 4.2 after this change. 😁

master (which should reflect 4-2-stable's performance):

Calculating -------------------------------------
                         2.410k i/100ms
-------------------------------------------------
                         28.280k (± 1.6%) i/s -    142.190k

4.1:

Calculating -------------------------------------
                         1.889k i/100ms
-------------------------------------------------
                         21.745k (± 4.2%) i/s -    109.562k

@printercu
Copy link
Contributor

👍 Thank you!

@mfazekas
Copy link
Contributor Author

Thanks! Our rspec suite with 4.2.1pre now about 20% faster than with rails 4.1 (vs 4.2.0 was being 30% slower)

@sgrif
Copy link
Contributor

sgrif commented Dec 23, 2014

❤️ !!!

sivagollapalli pushed a commit to sivagollapalli/rails that referenced this issue Dec 29, 2014
Calling `changed_attributes` will ultimately check if every mutable
attribute has changed in place. Since this gets called whenever an
attribute is assigned, it's extremely slow. Instead, we can avoid this
calculation until we actually need it.

Fixes rails#18029
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants