Skip to content

Commit

Permalink
Three performance improvements:
Browse files Browse the repository at this point in the history 
* for simple cases like User.last and User.order('name desc').last no need to perform Array#join operation.

* Instead of performing String#blank? do Array#empty?

* no need to create variable relation
  • Loading branch information
@tenderlove
Neeraj Singh authored and tenderlove committed Sep 27, 2010
1 parent e3d6434 commit fbd1d30
Showing 1 changed file with 9 additions and 10 deletions.
19 changes: 9 additions & 10 deletions activerecord/lib/active_record/relation/query_methods.rb
View file
Expand Up @@ -135,14 +135,13 @@ def extending(*modules, &block)
end

def reverse_order
order_clause = arel.order_clauses.join(', ')
relation = except(:order)
order_clause = arel.order_clauses

order = order_clause.blank? ?
order = order_clause.empty? ?
"#{@klass.table_name}.#{@klass.primary_key} DESC" :
reverse_sql_order(order_clause)
reverse_sql_order(order_clause).join(', ')

relation.order(Arel::SqlLiteral.new(order))
except(:order).order(Arel::SqlLiteral.new(order))
end

def arel
Expand Down Expand Up @@ -283,15 +282,15 @@ def apply_modules(modules)
end

def reverse_sql_order(order_query)
order_query.split(',').each { |s|
order_query.join(', ').split(',').collect { |s|
if s.match(/\s(asc|ASC)$/)
s.gsub!(/\s(asc|ASC)$/, ' DESC')
s.gsub(/\s(asc|ASC)$/, ' DESC')
elsif s.match(/\s(desc|DESC)$/)
s.gsub!(/\s(desc|DESC)$/, ' ASC')
s.gsub(/\s(desc|DESC)$/, ' ASC')
else
s.concat(' DESC')
s + ' DESC'
end
}.join(',')
}
end

def array_of_strings?(o)
Expand Down

12 comments on commit fbd1d30

@carlosantoniodasilva
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question, why removing gsub! and concat in reverse_sql_order?

@neerajsingh0101
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so that code like "created_at, updated_at" works with User.last. Note that the whole thing is a string. Hence we need to split by a comma.

@carlosantoniodasilva
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure Neeraj, but I believe it'd be possible to return the s variable inside the collect, keeping the methods that changes the actual string, would not?

@neerajsingh0101
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I am following you exactly. Can you post a gist or something?

@carlosantoniodasilva
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, here we go: http://gist.github.com/599566.
I also thought about some other possibilities to see what you think (didn't actually tested it), please take a look.

@thedarkone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about doing "Lazy gsub!": http://gist.github.com/599605?

@neerajsingh0101
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like the "lazy gsub!" . In all other cases first a regex check is being done and then gsub is being invoked. The "lazy gsub!" totally removes the need to do any regex check.

@carlosantoniodasilva
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agreed, nice one!

@neerajsingh0101
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the underlying assumption here is that gusb! is faster than regex. Unless there is benchmarking data I don't really know. May be someone from ruby core ( Aaron Patterson) might be able to tell which one is faster.

If regex turns out to be faster than gsub! then we should stick with regex approach. Otherwise switch to gsub! .

@thedarkone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably guess lazy gsub! to be slightly faster. I've also updated the gist (http://gist.github.com/599605) with a slightly better regexps, so somebody should run a bench and whip up a patch.

@lsylvester
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have run a benchmark at http://gist.github.com/600385

It looks like the lazy gsub! is faster.

          user     system      total        real
  orig:   1.660000   0.040000   1.700000 (  1.689658)
  mod1:   1.650000   0.030000   1.680000 (  1.693604)
  mod2:   1.370000   0.030000   1.400000 (  1.395524)
  mod3:   1.340000   0.020000   1.360000 (  1.358895)
  lazy:   0.930000   0.030000   0.960000 (  0.954206)

@neerajsingh0101
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm blown away by the result. That's significant perf improvement.

Please sign in to comment.