Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for major memory leak / slowdown proportionate to number of serviced requests. #466

Closed
wants to merge 4 commits into from

3 participants

@MrJoy

Prevent exponential increase in listeners. Works in Rails 3.0, and hopefully works in 3.1 too (I have not tested this, but the latest commit is in response to my last effort not working in Rails 3.1, and superficially should be robust).

@j-manu

Has this been tested ? Will it be merged?

@MrJoy

I've tested it against Rails 3.0 and it works well there. I've not tested against 3.1 as our app requires some significant work to bring up under 3.1. At a minimum, I would expect it to not be harmful in 3.1 but it would be nice if someone could pick this up and test it.

@MrJoy MrJoy closed this
@MrJoy MrJoy reopened this
@MrJoy

Whoops, didn't mean to close the request. >.<

@MrJoy

Did a quick test in 3.1, and there still appears to be memory leaks and significant per-request slowdowns even with this fix. That said, in some informal testing (hitting a static page where the controller does a GC.start, then looking at the process RSS, wash-rinse-repeat x100) this reduces average post-request RSS by 5.1MiB.

Behavior appears to be sane and correct from my superficial testing.

@MrJoy

Doing the same sort of test against our Rails 3.0 app and I see similar behavior -- this makes things better, and makes things degrade better (memory leaks only, not memory + time), but they still degrade.

So I suggest landing this but keep plugging away to find what's going on.

@gregbell
Owner

I've spent some time today looking into the memory issues, but haven't found the exact issue yet.

When I use the tooling you supplied on #170 I get:

module ActiveAdmin
  class Reloader
    def attach!
      STDERR.puts ">>>>>>>>>>>>>>>>>>>   ATTACH!"
      reloader_class.to_prepare do
        STDERR.puts ">>>>>>>>>>>>>>>>>>>   RELOAD!"
        ActiveAdmin.application.unload!
        Rails.application.reload_routes!
      end
    end
  end
end

I get the output:

=> Booting WEBrick
=> Rails 3.0.10 application starting in development on http://0.0.0.0:3000
=> Call with -d to detach
=> Ctrl-C to shutdown server
>>>>>>>>>>>>>>>>>>>   ATTACH!
>>>>>>>>>>>>>>>>>>>   RELOAD!
[2011-09-20 17:09:23] INFO  WEBrick 1.3.1
[2011-09-20 17:09:23] INFO  ruby 1.8.7 (2009-06-12) [i686-darwin11.0.0]
[2011-09-20 17:09:23] INFO  WEBrick::HTTPServer#start: pid=57789 port=3000
>>>>>>>>>>>>>>>>>>>   RELOAD!

Started GET "/admin/categories" for 127.0.0.1 at Tue Sep 20 17:09:27 -0700 2011
  Processing by Admin::CategoriesController#index as HTML
  SQL (0.6ms)   SELECT name
 FROM sqlite_master
 WHERE type = 'table' AND NOT name = 'sqlite_sequence'

  AdminUser Load (0.2ms)  SELECT "admin_users".* FROM "admin_users" WHERE "admin_users"."id" = 1 LIMIT 1
  Category Load (0.6ms)  SELECT "categories".* FROM "categories" ORDER BY "categories".id desc LIMIT 30 OFFSET 0
  SQL (0.1ms)  SELECT COUNT(*) FROM "categories"
  CACHE (0.0ms)  SELECT COUNT(*) FROM "categories"
Rendered /Users/gregbell/code/active_admin/app/views/active_admin/resource/index.html.arb (119.6ms)
Completed 200 OK in 158ms (Views: 126.5ms | ActiveRecord: 1.4ms)
>>>>>>>>>>>>>>>>>>>   RELOAD!

Which has 1 attach and then a reload on each page. Isn't this how the reloader should work? (This is before your patch)

Somewhere in Active Admin we're holding on to references of Namespaces that aren't being cleared out in Application#unload!

Are you getting different mileage?

@MrJoy

Definitely getting different mileage -- perhaps having to do with being on Ruby 1.9.2-p180?

@gregbell
Owner

Closing for now since memory issues seem to have been dealt with. Please open a new pull request if there are additional fixes that you would like to see.

@gregbell gregbell closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  features/support/env.rb
@@ -86,7 +86,7 @@
# Remove all our constants
Before do
- # We are cachine classes, but need to manually clear references to
+ # We are caching classes, but need to manually clear references to
# the controllers. If they aren't clear, the router stores references
ActiveSupport::Dependencies.clear
View
12 lib/active_admin.rb
@@ -13,10 +13,9 @@ module ActiveAdmin
autoload :VERSION, 'active_admin/version'
autoload :Application, 'active_admin/application'
autoload :AssetRegistration, 'active_admin/asset_registration'
- autoload :Breadcrumbs, 'active_admin/breadcrumbs'
+ autoload :BaseController, 'active_admin/base_controller'
autoload :Callbacks, 'active_admin/callbacks'
autoload :Component, 'active_admin/component'
- autoload :BaseController, 'active_admin/base_controller'
autoload :ControllerAction, 'active_admin/controller_action'
autoload :CSVBuilder, 'active_admin/csv_builder'
autoload :Dashboards, 'active_admin/dashboards'
@@ -25,26 +24,25 @@ module ActiveAdmin
autoload :Devise, 'active_admin/devise'
autoload :DSL, 'active_admin/dsl'
autoload :Event, 'active_admin/event'
- autoload :FormBuilder, 'active_admin/form_builder'
autoload :FilterFormBuilder, 'active_admin/filter_form_builder'
- autoload :Inputs, 'active_admin/inputs'
+ autoload :FormBuilder, 'active_admin/form_builder'
autoload :Iconic, 'active_admin/iconic'
+ autoload :Inputs, 'active_admin/inputs'
autoload :Menu, 'active_admin/menu'
autoload :MenuItem, 'active_admin/menu_item'
autoload :Namespace, 'active_admin/namespace'
autoload :Page, 'active_admin/page'
- autoload :PagePresenter, 'active_admin/page_presenter'
autoload :PageController, 'active_admin/page_controller'
autoload :PageDSL, 'active_admin/page_dsl'
+ autoload :PagePresenter, 'active_admin/page_presenter'
autoload :Reloader, 'active_admin/reloader'
+ autoload :Renderer, 'active_admin/renderer'
autoload :Resource, 'active_admin/resource'
autoload :ResourceController, 'active_admin/resource_controller'
autoload :ResourceDSL, 'active_admin/resource_dsl'
- autoload :Renderer, 'active_admin/renderer'
autoload :Scope, 'active_admin/scope'
autoload :ScopeChain, 'active_admin/helpers/scope_chain'
autoload :SidebarSection, 'active_admin/sidebar_section'
- autoload :TableBuilder, 'active_admin/table_builder'
autoload :ViewFactory, 'active_admin/view_factory'
autoload :ViewHelpers, 'active_admin/view_helpers'
autoload :Views, 'active_admin/views'
View
4 lib/active_admin/inputs.rb
@@ -5,10 +5,10 @@ module Inputs
autoload :DatepickerInput
autoload :FilterBase
- autoload :FilterStringInput
+ autoload :FilterCheckBoxesInput
autoload :FilterDateRangeInput
autoload :FilterNumericInput
autoload :FilterSelectInput
- autoload :FilterCheckBoxesInput
+ autoload :FilterStringInput
end
end
View
4 lib/active_admin/views/index_as_table.rb
@@ -164,8 +164,8 @@ def status_tag(*args, &block)
end
add_column col
end
- end # TableBuilder
+ end # IndexTableFor
- end # Table
+ end # IndexAsTable
end
end
Something went wrong with that request. Please try again.