When should we upgrade to Rails 4?


(tenderlove) #42

Ugh. Obviously Rails 4 needs a way to construct backwards compatible queries. You have no upgrade path here without major changes.

I’m not opposed to reverting that PR. I just don’t know if anyone is depending on it now. I think the most conservative route would be to revert the PR and add a new fix for 4.1 that does what I describe (in order to solve the original reporter’s issue). If other folks on the core team, then let’s go with adding the :prepend option.

Any other ideas?


(Sam Saffron) #43

I was thinking about this conundrum while eating my falafel.

I can not think of any use case where you would want to construct an ORDER BY clause in reverse, code like that is very hard to reason about. I support reverting this change.

I also do not think we should introduce magics that make you need to memorise a special rule for default scopes.

Why not simply add an unordered operator (you can even sneak it in to the next release):

    scope :adults, -> { where('age > 18').order('age') }
    users = User.adults
                .unordered
                .order('age desc')

That way if you get any AR query it is very easy to reason about ordering, and if you need to clobber the ordering you can easily do it.

Thoughts?


(tenderlove) #44

Ya, I’m fine with adding a method to completely clobber ordering. After many conversations with bitsweat about how default_scope is supposed to work, I’m still pretty certain that clobbering the order from a default_scope is the correct behavior. If we clobber where() keys, we should probably clobber orders (I guess).


(Sam Saffron) #45

At the moment the only place we use default_scope is in one spot in Discourse, and oh my god it is hacky, read it an cry :crying_cat_face:

https://github.com/discourse/discourse/blob/master/lib/trashable.rb

Is there a cleaner way of doing this? The clobbering really hoses us. We don’t want trashed topics to show up in default queries.


(Stephan) #46

The default scope is not the problem as far as I know.
Problem is code like this where multiple .order statements are chained.


(Sam Saffron) #47

Yeah my default_scope hack seems safe. Clobbering order on default_scope would not affect us, so I don’t really mind what happens there was just highlighting that the clobbering of where keys does affect us and we work around it.


(Sam Saffron) #48

Quick update, I had a chat with the core team. It looks like the order prepend stuff is going to be reverted in the next Rails 4 release. We talked about it for quite a while and the consensus is that we don’t want to stick to a decision we all regret. Better just deal with the breaking change. Rafael França said he will try to revert it this weekend, no promises.

I don’t mind if you monkey patch this back to non-prepend version in Discourse Rails 4, then when we upgrade to the next Rails 4 release we can revert the monkey patch.


(rafaelfranca) #49

Hey Guys! I just reverted that change on this commit. Revert change on ActiveRecord::Relation#order method that prepends new · rails/rails@609dae1 · GitHub

Could you try the 4-0-stable branch?


(Sam Saffron) #50

Rafael, Updated to 4-0-stable now and we are down to 4 failing specs … :dancers:

3 of the failing specs are due to the change that requires #includes be paired with #references in Rails 4, the last one I have not dug in to.

I am pretty sure @stephan is no longer blocked if he feels like giving it another shot :wink:

Thanks heaps


(Stephan) #51

What’s the next step? :smiley:


(Jeff Atwood) #52

Figuring out why the front page is 20% slower under Rails 4 than it currently is with Rails 3.


(Sam Saffron) #53

This may also interest @steveklabnik @tenderlove @novemberkilo and @rafaelfranca

I set up a reasonably simple process to get a “profile” instance of Discourse up.

http://meta.discourse.org/t/benchmarking-discourse-locally/9070

Using those steps, when I run in Rails 3 mode I get:

sam@ubuntu:~/Source/discourse$ ab -n 100 http://localhost:3000/  
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 localhost (be patient).....done


Server Software:        thin  
Server Hostname:        localhost  
Server Port:            3000

Document Path:          /  
Document Length:        46714 bytes

Concurrency Level:      1  
Time taken for tests:   3.337 seconds  
Complete requests:      100  
Failed requests:        0  
Write errors:           0  
Total transferred:      4701700 bytes  
HTML transferred:       4671400 bytes  
Requests per second:    29.97 [#/sec] (mean)  
Time per request:       33.366 [ms] (mean)  
Time per request:       33.366 [ms] (mean, across all concurrent requests)  
Transfer rate:          1376.09 [Kbytes/sec] received

Connection Times (ms)  
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0  
Processing:    27   33  10.6     30      82  
Waiting:       27   33  10.6     30      82  
Total:         27   33  10.6     31      82

Percentage of the requests served within a certain time (ms)  
  50%     31
  66%     31
  75%     32
  80%     33
  90%     35
  95%     76
  98%     78
  99%     82
 100%     82 (longest request)

When I run in Rails 4 mode I get:

sam@ubuntu:~/Source/discourse$ ab -n 100 http://localhost:3000/
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 localhost (be patient).....done


Server Software:        thin
Server Hostname:        localhost
Server Port:            3000

Document Path:          /
Document Length:        46963 bytes

Concurrency Level:      1
Time taken for tests:   3.831 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4739400 bytes
HTML transferred:       4696300 bytes
Requests per second:    26.10 [#/sec] (mean)
Time per request:       38.307 [ms] (mean)
Time per request:       38.307 [ms] (mean, across all concurrent requests)
Transfer rate:          1208.21 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    34   38   9.1     36      85
Waiting:       34   38   9.1     36      85
Total:         34   38   9.1     36      85

Percentage of the requests served within a certain time (ms)
  50%     36
  66%     37
  75%     37
  80%     37
  90%     38
  95%     42
  98%     83
  99%     85
 100%     85 (longest request)

This means the median request time for the front page in Rails 4 is raised from 31ms to 36ms. This is pretty significant and I would not like to move to Rails 4 until we figure out why this is happening (could be our fault) and get it fixed.

Yesterday I spent an hour profiling, I looked at flame graphs and query counts, the sad thing is that nothing really struck me as problematic. Only major oddity I found was that Rails 4 had a significantly higher in memory object count.


(Régis Hanol) #54

I’m seeing the same thing on the median request time for the front page:

  • 98ms for Rails 3
  • 120ms for Rails 4

(steveklabnik) #55

Bummer. :frowning: I haven’t seen a whole lot of benchmarks, but they’ve mostly been faster, not slower.


(Sam Saffron) #56

This could be a benchmarking or an upgrade issue


(Navin) #57

So I think I’ve fixed all remaining deprecation warnings.

But there is a spec failure on Rails 4 now that seems non-trivial. PostAnalyzer#cooked_document looks different under Rails3 and Rails4 - will investigate further.


(Navin) #58

@sam the issue appears to be with string encoding. In Rails3, Rails.cache.read returns strings encoded as UTF-8 but in Rails4, as ASCII-8BIT - so PostAnalyzer#cook is giving different results from a post with raw content containing a URL for a gif (for example). To reproduce the failure do:

 RAILS4=true bundle exec rspec spec/components/post_revisor_spec.rb:209

From several hours spent spelunking around today, this gist contains the salient bits of pry sessions (all around Oneboxer#render_from_cache) in Rails4 and Rails3 modes.

Run out of time today and figuring you might know a thing or two about the setup for Rails.cache and redis-store :wink:

Got any thoughts?


Update (28 Aug): Sorry to be the bearer of weird news but this appears to be a Heisenbug :unamused: @sam confirmed seeing this failure as well but all specs pass in Rails4 mode today. In the interim I’ve restarted my machine and done a RAILS4=true bundle install on current master and it’s gone.


(Sam Saffron) #59

Found another 2% of our 20% perf regression. We are slowly getting there.

https://github.com/rails/arel/pull/206


(Sam Saffron) #60

Another couple of fixes:

https://github.com/rails/rails/pull/12065

And

https://github.com/discourse/discourse/commit/46d5314ec482f76cf75e29bc22325bc0b4564aec

###Rails 3

---
home_page:
  50: 28
  75: 29
  90: 31
  99: 78
topic_page:
  50: 49
  75: 51
  90: 54
  99: 100
timings:
  load_rails: 2277
ruby-version: 2.0.0-p247
rails4?: false
architecture: amd64
operatingsystem: Ubuntu
kernelversion: 3.8.0
memorysize: 5.89 GB
physicalprocessorcount: 1
processor0: Intel(R) Core(TM) i7-4770K CPU @ 3.50GHz
virtual: vmware

###Rails 4

---
---
home_page:
  50: 33
  75: 33
  90: 35
  99: 79
topic_page:
  50: 47
  75: 49
  90: 50
  99: 99
timings:
  load_rails: 2389
ruby-version: 2.0.0-p247
rails4?: true
architecture: amd64
operatingsystem: Ubuntu
kernelversion: 3.8.0
memorysize: 5.89 GB
physicalprocessorcount: 1
processor0: Intel(R) Core(TM) i7-4770K CPU @ 3.50GHz
virtual: vmware

Turns out that one of my fixes to Discourse sped up Rails 3 quite a lot as well.


(Sam Saffron) #61

Update, I have been busy cleaning up all sorts of hotspots see:

I also improved the benching script to perform tests as admin as well.

Current results are:

##Rails 4

---
home_page:
  50: 27
  75: 28
  90: 29
  99: 77
topic_page:
  50: 45
  75: 47
  90: 51
  99: 94
home_page_admin:
  50: 36
  75: 37
  90: 38
  99: 85
topic_page_admin:
  50: 61
  75: 64
  90: 68
  99: 180
timings:
  load_rails: 2632
ruby-version: 2.0.0-p247
rails4?: true
architecture: amd64
operatingsystem: Ubuntu
kernelversion: 3.8.0
memorysize: 5.89 GB
physicalprocessorcount: 1
processor0: Intel(R) Core(TM) i7-4770K CPU @ 3.50GHz
virtual: vmware

Rails 3

---
home_page:
  50: 25
  75: 27
  90: 29
  99: 77
topic_page:
  50: 43
  75: 44
  90: 47
  99: 94
home_page_admin:
  50: 33
  75: 34
  90: 35
  99: 86
topic_page_admin:
  50: 64
  75: 68
  90: 75
  99: 166
timings:
  load_rails: 2471
ruby-version: 2.0.0-p247
rails4?: false
architecture: amd64
operatingsystem: Ubuntu
kernelversion: 3.8.0
memorysize: 5.89 GB
physicalprocessorcount: 1
processor0: Intel(R) Core(TM) i7-4770K CPU @ 3.50GHz
virtual: vmware

Many of my optimisations were applicable to Rails 3 as well, so our overall perf is better. Most importantly though the distance between Rails 3 and 4 is closing down.