Optimised performance for attribute extraction #233

SamSaffron · 2013-03-14T04:55:46Z

Benchmarks (in the benchmark folder) show these changes give 3x perf for serialization under some cases.

I the real world for a big discourse page we see a 10% boost (though it was a bit bigger on 2.0 for some reason):

BEFORE:

sam@ubuntu:~/Source/discourse$ ab -n 30 -c 1 http://l.discourse:3000/t/install-tutorial-for-the-ruby-dumb-test/2027
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking l.discourse (be patient).....done


Server Software:        thin
Server Hostname:        l.discourse
Server Port:            3000

Document Path:          /t/install-tutorial-for-the-ruby-dumb-test/2027
Document Length:        57264 bytes

Concurrency Level:      1
Time taken for tests:   5.182 seconds
Complete requests:      30
Failed requests:        6
   (Connect: 0, Receive: 0, Length: 6, Exceptions: 0)
Write errors:           0
Total transferred:      1744598 bytes
HTML transferred:       1717909 bytes
Requests per second:    5.79 [#/sec] (mean)
Time per request:       172.719 [ms] (mean)
Time per request:       172.719 [ms] (mean, across all concurrent requests)
Transfer rate:          328.80 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   143  173  22.4    179     215
Waiting:      143  173  22.4    179     215
Total:        143  173  22.4    179     215

Percentage of the requests served within a certain time (ms)
  50%    179
  66%    184
  75%    186
  80%    188
  90%    210
  95%    214
  98%    215
  99%    215
 100%    215 (longest request)

AFTER:

sam@ubuntu:~/Source/discourse$ ab -n 30 -c 1 http://l.discourse:3000/t/install-tutorial-for-the-ruby-dumb-test/2027
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking l.discourse (be patient).....done


Server Software:        thin
Server Hostname:        l.discourse
Server Port:            3000

Document Path:          /t/install-tutorial-for-the-ruby-dumb-test/2027
Document Length:        57097 bytes

Concurrency Level:      1
Time taken for tests:   4.911 seconds
Complete requests:      30
Failed requests:        12
   (Connect: 0, Receive: 0, Length: 12, Exceptions: 0)
Write errors:           0
Total transferred:      1739581 bytes
HTML transferred:       1712922 bytes
Requests per second:    6.11 [#/sec] (mean)
Time per request:       163.696 [ms] (mean)
Time per request:       163.696 [ms] (mean, across all concurrent requests)
Transfer rate:          345.93 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   140  164  25.9    145     231
Waiting:      140  164  25.9    145     231
Total:        140  164  25.9    145     231

Percentage of the requests served within a certain time (ms)
  50%    145
  66%    182
  75%    183
  80%    184
  90%    199
  95%    213
  98%    231
  99%    231
 100%    231 (longest request)

Disabled all instrumentation unless enabled explicitly

SamSaffron · 2013-03-14T04:56:45Z

Sorry ... squashed to commits into a different branch and stuff got messy ... anyway ... this should be good

steveklabnik · 2013-03-14T05:06:50Z

Awesome :)

steveklabnik · 2013-03-14T05:07:19Z

.gitignore

@@ -15,3 +15,4 @@ spec/reports
 test/tmp
 test/version_tmp
 tmp
+*.swp


ha, this is great, I have it in my global .gitignore as well as have vim configured to save these files elsewhere. :)

steveklabnik · 2013-03-14T05:08:50Z

I was talking with @wycats the other day, and we were discussing how there's probably too much instrumentation already, so I might be sympathetic to just removing it rather than doing all this config work.

steveklabnik · 2013-03-14T05:11:28Z

On My Machine:

$ bundle exec rake bench
{:id=>2, :name=>"sam", :age=>20, :about=>"about"}
{:id=>1, :name=>"sam", :about=>"about"}
Rehearsal --------------------------------------------------------------------------
init                                     0.070000   0.000000   0.070000 (  0.072378)
fast_hash                                0.130000   0.010000   0.140000 (  0.132188)
attributes                               0.330000   0.000000   0.330000 (  0.340366)
serializable_hash                        0.430000   0.000000   0.430000 (  0.432215)
serializable_hash_with_instrumentation   0.970000   0.020000   0.990000 (  0.984580)
----------------------------------------------------------------- total: 1.960000sec

                                             user     system      total        real
init                                     0.060000   0.000000   0.060000 (  0.060849)
fast_hash                                0.120000   0.000000   0.120000 (  0.116823)
attributes                               0.340000   0.010000   0.350000 (  0.340131)
serializable_hash                        0.430000   0.000000   0.430000 (  0.428502)
serializable_hash_with_instrumentation   0.940000   0.010000   0.950000 (  0.951877)

steveklabnik · 2013-03-14T05:12:43Z

lib/active_model/serializer.rb

+
+        if self.method_defined? :_fast_attributes
+          undef :_fast_attributes
+        end


Why do we need to define this over and over?

Oh, I guess the idea is that we write out a specific method call, attributes may have changed since the last time we did that?

yeah ... its just for extra safety, it only affects bootup anyway.

Right. I didn't even think about how it's only on boot. We shouldn't actually need it.

yeah, its just a safeguard if people re-open the class for whatever reason. one could argue that is an abuse of the api anyway

Even if they re-open the class, it shouldn't run this code, right? Just if they re-load this file.

I think we can remove this, and then I'll merge it in.

SamSaffron · 2013-03-14T05:13:28Z

Personally I prefer to remove this stuff, cause we can always inject instrumentation points with MiniProfiler when we need them. Or in my dream world use dtrace in linux .... for this kind of perf work in prd :)

steveklabnik · 2013-03-14T05:14:47Z

Sounds good. How about you just remove all of that?

SamSaffron · 2013-03-14T05:15:04Z

@steveklabnik may be worth running the bench (just comment out the serializable_hash_with_instrumentation ) against the old code base just to confirm my findings were correct.

SamSaffron · 2013-03-14T05:15:24Z

@steveklabnik sure ... let me change that ... one sec

steveklabnik · 2013-03-14T05:19:34Z

Reverting the changes made to serializer.rb

$ bundle exec rake bench
{:key=>"sam"}
{:key=>"sam"}
Rehearsal --------------------------------------------------------------------------
init                                     0.160000   0.000000   0.160000 (  0.161581)
fast_hash                                0.140000   0.010000   0.150000 (  0.148232)
attributes                               0.880000   0.000000   0.880000 (  0.876375)
serializable_hash                        0.880000   0.000000   0.880000 (  0.889685)
serializable_hash_with_instrumentation   1.160000   0.010000   1.170000 (  1.166031)
----------------------------------------------------------------- total: 3.240000sec

                                             user     system      total        real
init                                     0.060000   0.000000   0.060000 (  0.064159)
fast_hash                                0.120000   0.010000   0.130000 (  0.122550)
attributes                               0.540000   0.000000   0.540000 (  0.544504)
serializable_hash                        0.640000   0.000000   0.640000 (  0.635423)
serializable_hash_with_instrumentation   1.130000   0.010000   1.140000 (  1.138091)

Yeah, seems to be pretty dramatic.

teeparham · 2013-03-14T18:01:09Z

This is great! Huge 👍 to removing instrumentation altogether. If anyone wants instrumentation, they can add it as a separate gem.

Optimised performance for attribute extraction

steveklabnik · 2013-03-14T21:22:33Z

👍

SamSaffron · 2013-03-14T21:23:32Z

😯 thanks heaps!

steveklabnik · 2013-03-14T21:38:55Z

Thank YOU heaps ❤️

miloshadzic · 2013-03-14T23:00:43Z

@steveklabnik will you be cutting a new release soon with these recent patches? This pert work is awesome.

steveklabnik · 2013-03-14T23:08:55Z

Yes.

rails-api#230 rails-api#233 rails-api#242 rails-api#1305 rails-api#413 rails-api#1276

Optimised performance for attribute extraction

31ba6fb

Disabled all instrumentation unless enabled explicitly

steveklabnik reviewed Mar 14, 2013
View reviewed changes

per discussion remove instrumentation altogether

08a182d

remove safe guard per discussion with steveklabnik

710c375

steveklabnik added a commit that referenced this pull request Mar 14, 2013

Merge pull request #233 from SamSaffron/fix

6408b73

Optimised performance for attribute extraction

steveklabnik merged commit 6408b73 into rails-api:master Mar 14, 2013

bf4 mentioned this pull request Jun 21, 2015

Encapsulate serialization in ActiveModel::SerializableResource #954

Merged

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Dec 16, 2015

Add bench/perf.rb from 0.8 for reference

1c9a76a

rails-api#230 rails-api#233 rails-api#242 rails-api#1305 rails-api#413 rails-api#1276

bf4 mentioned this pull request Dec 23, 2015

Implement a method cache for faster dispatch and allowing super calls #849

Closed

bf4 mentioned this pull request Apr 22, 2018

Make to_json twice as fast as it used to be Netflix/fast_jsonapi#177

Closed

Optimised performance for attribute extraction #233

Optimised performance for attribute extraction #233

Uh oh!

Conversation

SamSaffron commented Mar 14, 2013

Uh oh!

SamSaffron commented Mar 14, 2013

Uh oh!

steveklabnik commented Mar 14, 2013

Uh oh!

steveklabnik Mar 14, 2013

Choose a reason for hiding this comment

Uh oh!

steveklabnik commented Mar 14, 2013

Uh oh!

steveklabnik commented Mar 14, 2013

Uh oh!

steveklabnik Mar 14, 2013

Choose a reason for hiding this comment

Uh oh!

steveklabnik Mar 14, 2013

Choose a reason for hiding this comment

Uh oh!

SamSaffron Mar 14, 2013

Choose a reason for hiding this comment

Uh oh!

steveklabnik Mar 14, 2013

Choose a reason for hiding this comment

Uh oh!

SamSaffron Mar 14, 2013

Choose a reason for hiding this comment

Uh oh!

steveklabnik Mar 14, 2013

Choose a reason for hiding this comment

Uh oh!

SamSaffron commented Mar 14, 2013

Uh oh!

steveklabnik commented Mar 14, 2013

Uh oh!

SamSaffron commented Mar 14, 2013

Uh oh!

SamSaffron commented Mar 14, 2013

Uh oh!

steveklabnik commented Mar 14, 2013

Uh oh!

teeparham commented Mar 14, 2013

Uh oh!

steveklabnik commented Mar 14, 2013

Uh oh!

SamSaffron commented Mar 14, 2013

Uh oh!

steveklabnik commented Mar 14, 2013

Uh oh!

miloshadzic commented Mar 14, 2013

Uh oh!

steveklabnik commented Mar 14, 2013

Uh oh!

Uh oh!