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

Reduced by one third allocated memory for #perform_async method #2288

Merged
merged 1 commit into from Apr 5, 2015

Conversation

3 participants
@davydovanton
Collaborator

davydovanton commented Apr 5, 2015

I used memory_profiler gem with this code:

MemoryProfiler.report{ 100.times { HardWorker.perform_async } }.pretty_print

Before (only for sidekiq)

Total allocated 22631
Total retained 886

allocated memory by gem
-----------------------------------
    318864  sidekiq

allocated objects by gem
-----------------------------------
      2416  sidekiq

retained memory by gem
-----------------------------------
      1280  sidekiq

retained objects by gem
-----------------------------------
        11  sidekiq

Allocated String Report
-----------------------------------
       307  "HardWorker"
       200  /Users/anton/work/gems/sidekiq/lib/sidekiq/client.rb:217

       300  "jid"
       200  /Users/anton/work/gems/sidekiq/lib/sidekiq/client.rb:223

       300  "enqueued_at"
       200  /Users/anton/work/gems/sidekiq/lib/sidekiq/client.rb:224

       101  "default"
         1  /Users/anton/work/gems/sidekiq/lib/sidekiq.rb:106

       100  "queue:default"
       100  /Users/anton/work/gems/sidekiq/lib/sidekiq/client.rb:196

       100  "get_sidekiq_options"
       100  /Users/anton/work/gems/sidekiq/lib/sidekiq/client.rb:215

After (only for sidekiq)

Total allocated 21249
Total retained 888

allocated memory by gem
-----------------------------------
    219312  sidekiq

allocated objects by gem
-----------------------------------
      1814  sidekiq

retained memory by gem
-----------------------------------
      1280  sidekiq

retained objects by gem
-----------------------------------
        11  sidekiq

Allocated String Report
-----------------------------------
       307  "HardWorker"
       200  /Users/anton/work/gems/sidekiq/lib/sidekiq/client.rb:217

       100  "queue:default"
       100  /Users/anton/work/gems/sidekiq/lib/sidekiq/client.rb:196
Show outdated Hide outdated lib/sidekiq/client.rb
Show outdated Hide outdated lib/sidekiq/worker.rb
@davydovanton

This comment has been minimized.

Show comment
Hide comment
@davydovanton

davydovanton Apr 5, 2015

Collaborator

@mperham @jonhyman fixed all, thanks for catch 👍

Collaborator

davydovanton commented Apr 5, 2015

@mperham @jonhyman fixed all, thanks for catch 👍

if url
options[:url] = url
end
options[:url] ||= determine_redis_provider

This comment has been minimized.

@mperham

mperham Apr 5, 2015

Owner

I'm worried this will change the logic slightly. The only difference is that it will create :url => nil if the ENV is not set but I don't know if the Redis client will work the same with it.

@mperham

mperham Apr 5, 2015

Owner

I'm worried this will change the logic slightly. The only difference is that it will create :url => nil if the ENV is not set but I don't know if the Redis client will work the same with it.

This comment has been minimized.

@davydovanton

davydovanton Apr 5, 2015

Collaborator

@mperham as I can see, url options uses only in log_info method:

if scrubbed_options[:url] && (uri = URI.parse(scrubbed_options[:url])) && uri.password
  uri.password = redacted
  scrubbed_options[:url] = uri.to_s
end

And I think that's not important to set :url => nil in option hash or not

@davydovanton

davydovanton Apr 5, 2015

Collaborator

@mperham as I can see, url options uses only in log_info method:

if scrubbed_options[:url] && (uri = URI.parse(scrubbed_options[:url])) && uri.password
  uri.password = redacted
  scrubbed_options[:url] = uri.to_s
end

And I think that's not important to set :url => nil in option hash or not

This comment has been minimized.

@davydovanton

davydovanton Apr 5, 2015

Collaborator

@mperham I find this. You can see if options[:url] equal nil redis connect to defaults[:url]. This shows that my changes are correct 🎉

@davydovanton

davydovanton Apr 5, 2015

Collaborator

@mperham I find this. You can see if options[:url] equal nil redis connect to defaults[:url]. This shows that my changes are correct 🎉

mperham added a commit that referenced this pull request Apr 5, 2015

Merge pull request #2288 from davydovanton/reduse-allocated
Reduced by one third allocated memory for #perform_async method

@mperham mperham merged commit df0a6cf into mperham:master Apr 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davydovanton davydovanton deleted the davydovanton:reduse-allocated branch Apr 5, 2015

@davydovanton

This comment has been minimized.

Show comment
Hide comment
@davydovanton

davydovanton Apr 5, 2015

Collaborator

thanks!

Collaborator

davydovanton commented Apr 5, 2015

thanks!

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Apr 5, 2015

Owner

Thank you for this. This is really awesome performance tuning.

Owner

mperham commented Apr 5, 2015

Thank you for this. This is really awesome performance tuning.

@davydovanton

This comment has been minimized.

Show comment
Hide comment
@davydovanton

davydovanton Apr 5, 2015

Collaborator

I hope that this performance tuning will be useful 🌟

Collaborator

davydovanton commented Apr 5, 2015

I hope that this performance tuning will be useful 🌟

@jonhyman

This comment has been minimized.

Show comment
Hide comment
@jonhyman

jonhyman Apr 5, 2015

Collaborator

🐎 😃

Collaborator

jonhyman commented Apr 5, 2015

🐎 😃

@davydovanton

This comment has been minimized.

Show comment
Hide comment
@davydovanton

davydovanton Apr 6, 2015

Collaborator

@mperham sorry, but I forgot to add this changes to CHANGELOG 😓
May be I need update this?

Collaborator

davydovanton commented Apr 6, 2015

@mperham sorry, but I forgot to add this changes to CHANGELOG 😓
May be I need update this?

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Apr 6, 2015

Owner

I typically only put items in the changelog that might be relevant to users (API change, possible breakage, etc). I don't think this improvement qualifies.

Owner

mperham commented Apr 6, 2015

I typically only put items in the changelog that might be relevant to users (API change, possible breakage, etc). I don't think this improvement qualifies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment