Improve memory usage #820

seanlinsley · 2017-08-02T20:45:37Z

Proof-of-concept for #819

These changes decreased memory usage by 96% 🚀 (1.3 MB to 58 KB)

Total allocated: 59848 bytes (1164 objects)
Total retained:  712 bytes (14 objects)

allocated memory by location
-----------------------------------
      8048  ransack/lib/ransack/adapters/active_record/ransack/context.rb:11
      7056  ransack/lib/ransack/adapters/active_record/context.rb:55
      4080  ransack/lib/ransack/adapters/active_record/context.rb:49
      3024  ransack/lib/ransack/adapters/active_record/context.rb:50
      2872  ransack/lib/ransack/adapters/active_record/context.rb:254
      2680  ransack/lib/ransack/adapters/active_record/context.rb:268
      2680  ransack/lib/ransack/adapters/active_record/context.rb:271

allocated objects by location
-----------------------------------
       102  ransack/lib/ransack/adapters/active_record/context.rb:49
        68  ransack/lib/ransack/adapters/active_record/context.rb:254
        67  ransack/lib/ransack/adapters/active_record/context.rb:268
        67  ransack/lib/ransack/adapters/active_record/context.rb:271
        67  ransack/lib/ransack/adapters/active_record/context.rb:272
        67  ransack/lib/ransack/adapters/active_record/context.rb:278
        67  ransack/lib/ransack/adapters/active_record/context.rb:284

ernie · 2017-08-02T20:56:40Z

lib/ransack/adapters/active_record/base.rb

@@ -45,7 +45,7 @@ def ransackable_attributes(auth_object = nil)
        # For overriding with a whitelist array of strings.
        #
        def ransackable_associations(auth_object = nil)
-          reflect_on_all_associations.map { |a| a.name.to_s }
+          @ransackable_associations ||= reflect_on_all_associations.map { |a| a.name.to_s }


You'll want to be careful about memoization in these -- that's why they weren't to begin with. Technically, new values could be added by code at runtime -- they would need to invalidate the cache.

Yeah, I hesitated to make this change. This PR isn't meant to be mergeable in its current state, I just went through and applied optimizations to the code that the report highlighted.

Now that we have this proof-of-concept, we can discuss which changes make sense.

Possibly:

model_associations = reflect_on_all_associations if @ransackable_associations && @ransackable_associations.size == model_associations.size @ransackable_associations else @ransackable_associations = model_associations.map { |a| a.name.to_s } end

Technically, new values could be added by code at runtime -- they would need to invalidate the cache.

Depending on your definition of "runtime", PaperTrail does this (adds associations to your models).

It appears that we can use the to_prepare Rails callback to expire the cache, though that would require that we do one of these:

instead of using instance variables, cache the values in a global object

cache the values in instance variables, but via a module that would hook into to_prepare

It doesn't look like we have tests to verify that Rails code reloading works with Ransack. IIRC ActiveAdmin has tests of that type, but they require a lot of setup code.

Actually to_prepare is unnecessary since these methods are defined on the models themselves. In the Paper Trail case, adding has_paper_trail to the model would unload & reload the model, so ransackable_associations is no longer cached.

AFAICT there aren't any scenarios where this could cause a bug.

seanlinsley · 2017-08-02T23:03:22Z

lib/ransack/adapters/active_record/ransack/context.rb


-      @default_table = Arel::Table.new(
-        @base.table_name, as: @base.aliased_table_name, type_caster: self
-        )


This variable didn't seem to be used anywhere and it represented a lot of memory usage, so I removed it.

seanlinsley · 2017-08-02T23:06:40Z

lib/ransack/adapters/active_record/ransack/context.rb

+      @bind_pairs ||= {}
+
+      @bind_pairs[key] ||= begin
+        parent, attr_name = get_parent_and_attribute_name(key.to_s)


It's not clear to me why this would help (other than not initializing a hash until a bind pair is actually needed), but it represented 18 KB, while the line above it represented 30 KB.

seanlinsley · 2017-08-02T23:12:45Z

lib/ransack/configuration.rb

+      end
+    end
+
+    self.predicates = PredicateCollection.new


This optimization makes it so that we don't have to re-sort the predicate array numerous times, and so that we don't have to re-allocate strings for checking end_with? in predicate.rb.

This change has the biggest impact, saving 806 KB (4 of the top 5 offenders)

The problem is, this code assumes that no object will modify the predicates hash in-place.

It also assumes that the same predicate key is never set twice.

seanlinsley · 2017-08-03T20:16:38Z

lib/ransack/context.rb

@@ -135,7 +135,7 @@ def ransackable_association?(str, klass)
    end

    def ransackable_scope?(str, klass)
-      klass.ransackable_scopes(auth_object).any? { |s| s.to_s == str }
+      klass.ransackable_scopes(auth_object).any? { |s| s.to_sym == str.to_sym }


We can assume that most people use symbols instead of strings, so this prevents 13 KB of string allocations without sacrificing backward compatibility.

Depending on the ruby version, this could cause a memory leak from different string inputs being added to the symbol table.

Search criteria can come from serialized JSON or params, can we really assume most inputs are symbols?

That's Ruby 1.8 and below if I recall correctly, and Ransack currently requires 1.9 or above. (However note that on another line I'm using Ruby 2 keyword arguments).

I want to say it was in the 2.1 series, too, but I could be wrong.

http://www.cvedetails.com/cve/CVE-2013-1854/

Yes, Symbol GC was added in ruby 2.2. This may be OK now, depending on what Ransack wants to support.

ref #857 CC @seanfcarroll

colesteaks · 2017-12-19T21:42:35Z

@seanlinsley any updates here?

seanlinsley · 2017-12-19T23:50:49Z

I haven't focused on this in quite a while. If you're interested in contributing @colesteaks, I'd be happy to pair with you on this sometime.

seanlinsley · 2017-12-31T21:07:12Z

I'm going to start using this in my app to see if any bugs appear.

scarroll32 · 2018-01-02T23:42:17Z

Happy to merge this into master if it's got consensus. Thoughts?

seanlinsley · 2018-01-02T23:56:50Z

My only concern is that the test suite might not have caught the type of bugs that real Rails apps would surface.

seanlinsley · 2018-01-09T16:09:35Z

lib/ransack/predicate.rb

@@ -9,34 +9,26 @@ def names
        Ransack.predicates.keys
      end

-      def names_by_decreasing_length
-        names.sort { |a, b| b.length <=> a.length }
-      end


ActiveAdmin relies on this method:

NoMethodError: undefined method 'names_by_decreasing_length' for Ransack::Predicate:Class active_admin/filters/humanized.rb in predicates at line 50 end def predicates Ransack::Predicate.names_by_decreasing_length end def ransack_predicate_translation active_admin/filters/humanized.rb in current_predicate at line 46 end def current_predicate @current_predicate ||= predicates.detect { |p| @body.end_with?("_#{p}") } end def predicates active_admin/filters/humanized.rb in ransack_predicate_translation at line 54 end def ransack_predicate_translation I18n.t("ransack.predicates.#{current_predicate}") end def active_admin_predicate_translation active_admin/filters/humanized.rb in body at line 17 end def body predicate = ransack_predicate_translation if current_predicate.nil? predicate = @body.titleize active_admin/filters/resource_extension.rb in block (6 levels) in search_status_section at line 170 else active.filters.each do |filter| li do span filter.body b filter.value end end

It seems like ActiveAdmin was duplicating behavior that Ransack already provided, so it could be simplified to:

def current_predicate Ransack::Predicate.detect_from_string @body end

Ah, activeadmin/activeadmin#4951 replaced this code with Ransack::Translate.predicate

seanlinsley · 2018-01-10T23:08:40Z

After also upgrading ActiveAdmin from 1.0 to 1.2, I'm now using this patch in production.

scarroll32 · 2018-01-12T12:26:54Z

I vote we merge it. I'll do so over the weekend unless anyone disagrees.

seanlinsley · 2018-01-12T19:35:17Z

We haven't seen any issues so far 👍

seanlinsley · 2018-01-18T03:31:24Z

Still no issues, so this should be good to go.

seb-sykio · 2018-11-19T12:39:09Z

hi
I'm still having this issue "exception : undefined method `names_by_decreasing_length' for Ransack::Predicate:Class"
I'm using ransack (2.1.0) and activeadmin (1.4.0)

thanks for help

…memory-usage Improve memory usage

ernie reviewed Aug 2, 2017

View reviewed changes

seanlinsley commented Aug 2, 2017

View reviewed changes

seanlinsley commented Aug 3, 2017

View reviewed changes

seanlinsley force-pushed the improve-memory-usage branch from 79ae30f to d5ea5ce Compare December 29, 2017 18:36

seanlinsley changed the title ~~WIP: Improve memory usage~~ Improve memory usage Dec 29, 2017

seanlinsley force-pushed the improve-memory-usage branch from d5ea5ce to ebd159f Compare December 31, 2017 21:01

improve memory usage

f570a40

seanlinsley force-pushed the improve-memory-usage branch from ebd159f to f570a40 Compare January 1, 2018 02:31

seanlinsley commented Jan 9, 2018

View reviewed changes

seanlinsley mentioned this pull request Jan 10, 2018

Improving memory efficiency #819

Closed

scarroll32 merged commit feaebd5 into activerecord-hackery:master Jan 23, 2018

seanlinsley deleted the improve-memory-usage branch January 25, 2018 23:59

seanlinsley mentioned this pull request Jan 29, 2018

Fix left_outer_joins not being respected #764

Merged

seanlinsley mentioned this pull request Feb 15, 2018

undefined method `map' for #<Ransack::Configuration::PredicateCollection: #879

Closed

varyonic mentioned this pull request Nov 13, 2018

Removal of Ransack::Predicates.names_by_decreasing_length breaks ActiveAdmin 1.0 #888

Closed

tilvin mentioned this pull request Nov 15, 2020

Issue with Ransack 1.8.6 activeadmin/activeadmin#5333

Closed

tekniklr pushed a commit to tekniklr/ransack that referenced this pull request Mar 19, 2021

Merge pull request activerecord-hackery#820 from seanlinsley/improve-…

68201d3

…memory-usage Improve memory usage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve memory usage #820

Improve memory usage #820

seanlinsley commented Aug 2, 2017 •

edited

Loading

ernie Aug 2, 2017

seanlinsley Aug 2, 2017 •

edited

Loading

avit Aug 2, 2017 •

edited

Loading

jaredbeck Aug 2, 2017

seanlinsley Sep 17, 2017

seanlinsley Dec 29, 2017 •

edited

Loading

seanlinsley Dec 31, 2017

seanlinsley Aug 2, 2017

seanlinsley Aug 2, 2017

seanlinsley Aug 2, 2017

seanlinsley Aug 2, 2017

seanlinsley Aug 2, 2017

seanlinsley Aug 3, 2017

avit Aug 3, 2017 •

edited

Loading

seanlinsley Aug 3, 2017

ernie Aug 3, 2017

avit Aug 3, 2017

avit Aug 3, 2017

seanlinsley Dec 29, 2017

colesteaks commented Dec 19, 2017

seanlinsley commented Dec 19, 2017

seanlinsley commented Dec 31, 2017

scarroll32 commented Jan 2, 2018

seanlinsley commented Jan 2, 2018

seanlinsley Jan 9, 2018

seanlinsley Jan 9, 2018

seanlinsley Jan 9, 2018

seanlinsley commented Jan 10, 2018

scarroll32 commented Jan 12, 2018

seanlinsley commented Jan 12, 2018

seanlinsley commented Jan 18, 2018

seb-sykio commented Nov 19, 2018

Improve memory usage #820

Improve memory usage #820

Conversation

seanlinsley commented Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

seanlinsley Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

avit Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlinsley Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avit Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesteaks commented Dec 19, 2017

seanlinsley commented Dec 19, 2017

seanlinsley commented Dec 31, 2017

scarroll32 commented Jan 2, 2018

seanlinsley commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlinsley commented Jan 10, 2018

scarroll32 commented Jan 12, 2018

seanlinsley commented Jan 12, 2018

seanlinsley commented Jan 18, 2018

seb-sykio commented Nov 19, 2018

seanlinsley commented Aug 2, 2017 •

edited

Loading

seanlinsley Aug 2, 2017 •

edited

Loading

avit Aug 2, 2017 •

edited

Loading

seanlinsley Dec 29, 2017 •

edited

Loading

avit Aug 3, 2017 •

edited

Loading