Skip to content
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.

Sign up for GitHub

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

Improve memory usage #820

Merged
merged 1 commit into from
Jan 23, 2018
Merged

Improve memory usage #820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/ransack/adapters/active_record/3.0/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def attribute_method?(str, klass = @klass)

if ransackable_attribute?(str, klass)
exists = true
elsif (segments = str.split(/_/)).size > 1
elsif (segments = str.split(Constants::UNDERSCORE)).size > 1
remainder = []
found_assoc = nil
while !found_assoc && remainder.unshift(segments.pop) &&
Expand Down Expand Up @@ -98,7 +98,7 @@ def get_parent_and_attribute_name(str, parent = @base)

if ransackable_attribute?(str, klassify(parent))
attr_name = str
elsif (segments = str.split(/_/)).size > 1
elsif (segments = str.split(Constants::UNDERSCORE)).size > 1
remainder = []
found_assoc = nil
while remainder.unshift(segments.pop) && segments.size > 0 &&
Expand Down
4 changes: 2 additions & 2 deletions lib/ransack/adapters/active_record/3.1/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def attribute_method?(str, klass = @klass)

if ransackable_attribute?(str, klass)
exists = true
elsif (segments = str.split(/_/)).size > 1
elsif (segments = str.split(Constants::UNDERSCORE)).size > 1
remainder = []
found_assoc = nil
while !found_assoc && remainder.unshift(segments.pop) &&
Expand Down Expand Up @@ -105,7 +105,7 @@ def get_parent_and_attribute_name(str, parent = @base)

if ransackable_attribute?(str, klassify(parent))
attr_name = str
elsif (segments = str.split(/_/)).size > 1
elsif (segments = str.split(Constants::UNDERSCORE)).size > 1
remainder = []
found_assoc = nil
while remainder.unshift(segments.pop) && segments.size > 0 &&
Expand Down
4 changes: 2 additions & 2 deletions lib/ransack/adapters/active_record/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def ransack_alias(new_name, old_name)
# For overriding with a whitelist array of strings.
#
def ransackable_attributes(auth_object = nil)
if Ransack::SUPPORTS_ATTRIBUTE_ALIAS
@ransackable_attributes ||= if Ransack::SUPPORTS_ATTRIBUTE_ALIAS
column_names + _ransackers.keys + _ransack_aliases.keys +
attribute_aliases.keys
else
Expand All @@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@seanlinsley seanlinsley Aug 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@avit avit Aug 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@seanlinsley seanlinsley Dec 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

end

# Ransortable_attributes, by default, returns the names
Expand Down
10 changes: 5 additions & 5 deletions lib/ransack/adapters/active_record/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def attribute_method?(str, klass = @klass)
exists = false
if ransackable_attribute?(str, klass)
exists = true
elsif (segments = str.split(/_/)).size > 1
elsif (segments = str.split(Constants::UNDERSCORE)).size > 1
remainder = []
found_assoc = nil
while !found_assoc && remainder.unshift(segments.pop) &&
Expand Down Expand Up @@ -268,7 +268,8 @@ def build_joins(relation)
association_joins = buckets[:association_join]
stashed_association_joins = buckets[:stashed_join]
join_nodes = buckets[:join_node].uniq
string_joins = buckets[:string_join].map(&:strip).uniq
string_joins = buckets[:string_join].map(&:strip)
string_joins.uniq!

join_list =
if ::ActiveRecord::VERSION::MAJOR >= 5
Expand All @@ -295,10 +296,9 @@ def build_joins(relation)
end

def convert_join_strings_to_ast(table, joins)
joins.map! { |join| table.create_string_join(Arel.sql(join)) unless join.blank? }
joins.compact!
joins
.flatten
.reject(&:blank?)
.map { |join| table.create_string_join(Arel.sql(join)) }
end

def build_or_find_association(name, parent = @base, klass = nil)
Expand Down
15 changes: 7 additions & 8 deletions lib/ransack/adapters/active_record/ransack/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,14 @@ def initialize(object, options = {})
@base = @join_dependency.join_base
@engine = @base.arel_engine
end
end

@default_table = Arel::Table.new(
@base.table_name, as: @base.aliased_table_name, type_caster: self
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@bind_pairs = Hash.new do |hash, key|
parent, attr_name = get_parent_and_attribute_name(key)
if parent && attr_name
hash[key] = [parent, attr_name]
end
def bind_pair_for(key)
@bind_pairs ||= {}

@bind_pairs[key] ||= begin
parent, attr_name = get_parent_and_attribute_name(key.to_s)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

[parent, attr_name] if parent && attr_name
end
end

Expand Down
6 changes: 3 additions & 3 deletions lib/ransack/adapters/mongoid/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def type_for(attr)

name = '_id' if name == 'id'

t = object.klass.fields[name].try(:type) || @bind_pairs[attr.name].first.fields[name].type
t = object.klass.fields[name].try(:type) || bind_pair_for(attr.name).first.fields[name].type

t.to_s.demodulize.underscore.to_sym
end
Expand All @@ -61,7 +61,7 @@ def attribute_method?(str, klass = @klass)
exists = false
if ransackable_attribute?(str, klass)
exists = true
elsif (segments = str.split(/_/)).size > 1
elsif (segments = str.split(Constants::UNDERSCORE)).size > 1
remainder = []
found_assoc = nil
while !found_assoc && remainder.unshift(
Expand Down Expand Up @@ -111,7 +111,7 @@ def get_parent_and_attribute_name(str, parent = @base)

if ransackable_attribute?(str, klassify(parent))
attr_name = str
elsif (segments = str.split(/_/)).size > 1
elsif (segments = str.split(Constants::UNDERSCORE)).size > 1
remainder = []
found_assoc = nil
while remainder.unshift(
Expand Down
13 changes: 6 additions & 7 deletions lib/ransack/adapters/mongoid/ransack/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,14 @@ def initialize(object, options = {})

@base = @object.klass
# @engine = @base.arel_engine
end

def bind_pair_for(key)
@bind_pairs ||= {}

# @default_table = Arel::Table.new(
# @base.table_name, :as => @base.aliased_table_name, :engine => @engine
# )
@bind_pairs = Hash.new do |hash, key|
@bind_pairs[key] ||= begin
parent, attr_name = get_parent_and_attribute_name(key.to_s)
if parent && attr_name
hash[key] = [parent, attr_name]
end
[parent, attr_name] if parent && attr_name
end
end

Expand Down
22 changes: 21 additions & 1 deletion lib/ransack/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,27 @@ module Ransack
module Configuration

mattr_accessor :predicates, :options
self.predicates = {}

class PredicateCollection
attr_reader :sorted_names_with_underscores

def initialize
@collection = {}
@sorted_names_with_underscores = []
end

delegate :[], :keys, :has_key?, to: :@collection

def []=(key, value)
@sorted_names_with_underscores << [key, '_' + key]
@sorted_names_with_underscores.sort! { |(a, _), (b, _)| b.length <=> a.length }

@collection[key] = value
end
end

self.predicates = PredicateCollection.new
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


self.options = {
:search_key => :q,
:ignore_unknown_conditions => true,
Expand Down
16 changes: 8 additions & 8 deletions lib/ransack/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def klassify(obj)
# Convert a string representing a chain of associations and an attribute
# into the attribute itself
def contextualize(str)
parent, attr_name = @bind_pairs[str]
parent, attr_name = bind_pair_for(str)
table_for(parent)[attr_name]
end

Expand All @@ -59,24 +59,24 @@ def scope_arity(scope)

def bind(object, str)
return nil unless str
object.parent, object.attr_name = @bind_pairs[str]
object.parent, object.attr_name = bind_pair_for(str)
end

def traverse(str, base = @base)
str ||= ''.freeze

if (segments = str.split(/_/)).size > 0
if (segments = str.split(Constants::UNDERSCORE)).size > 0
remainder = []
found_assoc = nil
while !found_assoc && segments.size > 0 do
# Strip the _of_Model_type text from the association name, but hold
# onto it in klass, for use as the next base
assoc, klass = unpolymorphize_association(
segments.join('_'.freeze)
segments.join(Constants::UNDERSCORE)
)
if found_assoc = get_association(assoc, base)
base = traverse(
remainder.join('_'.freeze), klass || found_assoc.klass
remainder.join(Constants::UNDERSCORE), klass || found_assoc.klass
)
end

Expand All @@ -93,9 +93,9 @@ def association_path(str, base = @base)
base = klassify(base)
str ||= ''.freeze
path = []
segments = str.split(/_/)
segments = str.split(Constants::UNDERSCORE)
association_parts = []
if (segments = str.split(/_/)).size > 0
if (segments = str.split(Constants::UNDERSCORE)).size > 0
while segments.size > 0 &&
!base.columns_hash[segments.join(Constants::UNDERSCORE)] &&
association_parts << segments.shift do
Expand Down Expand Up @@ -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 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@avit avit Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref #857 CC @seanfcarroll

end

def searchable_attributes(str = ''.freeze)
Expand Down
2 changes: 1 addition & 1 deletion lib/ransack/nodes/grouping.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ def read_attribute(name)
end

def strip_predicate_and_index(str)
string = str.split(/\(/).first
string = str[/(.+?)\(/, 1] || str.dup
Predicate.detect_and_strip_from_string!(string)
string
end
Expand Down
30 changes: 11 additions & 19 deletions lib/ransack/predicate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,34 +9,26 @@ def names
Ransack.predicates.keys
end

def names_by_decreasing_length
names.sort { |a, b| b.length <=> a.length }
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


def named(name)
Ransack.predicates[name.to_s]
end

def detect_and_strip_from_string!(str)
if p = detect_from_string(str)
str.sub! /_#{p}$/, ''.freeze
p
end
detect_from_string str, chomp: true
end

def detect_from_string(str)
names_by_decreasing_length.detect { |p| str.end_with?("_#{p}") }
end
def detect_from_string(str, chomp: false)
return unless str

# def name_from_attribute_name(attribute_name)
# names_by_decreasing_length.detect {
# |p| attribute_name.to_s.match(/_#{p}$/)
# }
# end
Ransack.predicates.sorted_names_with_underscores.each do |predicate, underscored|
if str.end_with? underscored
str.chomp! underscored if chomp
return predicate
end
end

# def for_attribute_name(attribute_name)
# self.named(detect_from_string(attribute_name.to_s))
# end
nil
end

end

Expand Down
4 changes: 2 additions & 2 deletions lib/ransack/translate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ def self.attribute(key, options = {})
base_ancestors = base_class.ancestors.select {
|x| x.respond_to?(:model_name)
}
predicate = Predicate.detect_from_string(original_name)
attributes_str = original_name.sub(/_#{predicate}$/, ''.freeze)
attributes_str = original_name.dup # will be modified by ⬇
predicate = Predicate.detect_and_strip_from_string!(attributes_str)
attribute_names = attributes_str.split(/_and_|_or_/)
combinator = attributes_str.match(/_and_/) ? :and : :or
defaults = base_ancestors.map do |klass|
Expand Down