Ask A Question


You’re not receiving notifications from this thread.

[Performance] - Can I Still Refactor This Code?

Zulhilmi Zainudin asked in Rails

I'm building an API based Rails app and I've one endpoint to list down all active users account numbers and gold balance inside that account.

The requirement is:

  1. Take all active users (kyc_passed: true) and not admin users (admin: false)
  2. For each user, take his/her gold balance amount and trucate
  3. Sort by account number (ascending)

I delegate and put all the logics inside User model. But I found this query still using lots of objects. AppSignal report for 1500+ users:

As you can see above, 66K objects were allocated for this sample.

I believe this query can be improved so that less objects are allocated for each request. But I'm out of idea already. I try to switch between .includes and .joins but I can't see significant improvements.

And one more thing, I didn't have luck with .select. That's why I'm using .pluck.

Please advice how can I improve this query. I truly appreciate your help.

Models / table columns:

All non-relevant columns were omitted for readability.

class User < ActiveRecord::Base {
  :id             => :integer,
  :admin          => :boolean,
  :kyc_passed     => :boolean,
  :account_number => :string

class Balance < ActiveRecord::Base {
  :id             => :integer,
  :user_id        => :integer,
  :gateway_id     => :integer,
  :amount         => :decimal

class Gateway < ActiveRecord::Base {
  :id             => :integer,
  :currency       => :string

Example result:



#  app/controllers/api_controller.rb
def cust_gold_balances
    data = User.all_users_gold_balances
    render :json => {result: "ok", data: data}
#  app/models/user.rb
scope :active_users, -> {where(kyc_passed: true, admin: false)

def self.all_users_gold_balances
    gold_gateway_id = Gateway.find_by(currency: "GLD").id
    user_balances   = Balance.where(gateway_id: gold_gateway_id).includes(:user).where(user: active_users).pluck('balances.amount', 'users.account_number')
    array           = []
    user_balances.each {|balance| array << {account_number: balance[1], gold_balance: balance[0].truncate(3)} }
    return array.sort_by {|hash| hash[:account_number]}

I think you're generally going to struggle optimizing the number of allocations in this because your operation happens on every single user balance. It's going to allocate a ton of objects no matter what you do since you're loading every active one.

One optimization you can make is to pluck the active user IDs and query by that rather than loading up all those objects. I would also test to see if the in place mutation methods improve performance like so:

def self.all_users_gold_balances
  gold_gateway_id = Gateway.find_by(currency: "GLD").id
  active_user_ids = active_users.pluck(:id)
  user_balances   = Balance.where(gateway_id: gold_gateway_id).includes(:user).where(user: active_user_ids).pluck('balances.amount', 'users.account_number')! {|balance| array << {account_number: balance[1], gold_balance: balance[0].truncate(3)} }
  user_balances.sort_by! {|hash| hash[:account_number]}
  return user_balances

Thanks Chris. What do you mean by in place mutation methods improve performance?

Can you elaborate more and give some examples?



Basically those methods with ! at the end of them, they actually mutate (or modify) the variable in place. Without the ! a map will create a new array in memory so you'd have to full arrays in memory, and you could discard the other one after the method completes. Using the ! methods should improve your object allocations because they won't be creating new arrays in memory.


Got it. I'll give it a try!

Join the discussion
Create an account Log in

Want to stay up-to-date with Ruby on Rails?

Join 83,038+ developers who get early access to new tutorials, screencasts, articles, and more.

    We care about the protection of your data. Read our Privacy Policy.