Performance fixes #457

Merged
merged 2 commits into from Sep 28, 2012
View
8 lib/mongo_mapper/extensions/time.rb 100644 → 100755
@@ -3,18 +3,18 @@ module MongoMapper
module Extensions
module Time
def to_mongo(value)
- if value.nil? || value == ''
+ if !value.present?
nil
else
- time_class = ::Time.try(:zone).present? ? ::Time.zone : ::Time
+ time_class = ::Time.try(:zone) || ::Time
time = value.is_a?(::Time) ? value : time_class.parse(value.to_s)
at(time.to_f).utc if time # ensure milliseconds are preserved with to_f (issue #308)
end
end
def from_mongo(value)
- if ::Time.try(:zone).present? && value.present?
- value.in_time_zone(::Time.zone)
+ if value and zone = ::Time.try(:zone)
+ value.in_time_zone(zone)
else
value
end
View
0 lib/mongo_mapper/plugins/dirty.rb 100644 → 100755
File mode changed.
View
26 lib/mongo_mapper/plugins/keys.rb 100644 → 100755
@@ -181,7 +181,7 @@ def persisted?
end
def attributes=(attrs)
- return if attrs.blank?
+ return if attrs == nil or attrs.blank?
@jnunemaker
jnunemaker added a line comment Sep 28, 2012

nil and blank are the same thing. How much of a gain do we get from short circuiting the nil check?

@cheald
MongoMapper member
cheald added a line comment Sep 28, 2012

They are the same effect, but blank? is implemented in Ruby, whereas the nil check stays in C. In general, I'm trying to avoid ActiveSupport candy and go more "to the metal".

require 'benchmark'
require 'active_support/core_ext/string'

times = 10000000

foo = "asdf"

Benchmark.bm do |x|
  x.report("nil") { times.times { foo.nil? or foo.length == 0 }}
  x.report("blank?") { times.times { foo.blank? }}
end

   user     system      total        real
nil  1.010000   0.000000   1.010000 (  1.008783)
blank?  8.050000   0.000000   8.050000 (  8.096276)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
attrs.each_pair do |key, value|
if respond_to?(:"#{key}=")
@@ -244,10 +244,16 @@ def id=(value)
self[:_id] = value
end
- def [](name)
- read_key(name)
+ def read_key(key_name)
@jnunemaker
jnunemaker added a line comment Sep 28, 2012

This makes sense. Fewer actual methods calls.

@cheald
MongoMapper member
cheald added a line comment Sep 28, 2012

I did have to change the visibility of #read_key to achieve that, but the performance gain is worth a wee bit of code smell, IMO. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if key = keys[key_name.to_s]
+ value = key.get(instance_variable_get(:"@#{key_name}"))
+ set_parent_document(key, value) if key.embeddable?
+ instance_variable_set(:"@#{key_name}", value)
+ end
end
+ alias_method :[], :read_key
+
def []=(name, value)
ensure_key_exists(name)
write_key(name, value)
@@ -271,7 +277,7 @@ def embedded_keys
private
def load_from_database(attrs)
- return if attrs.blank?
+ return if attrs == nil or attrs.blank?
attrs.each do |key, value|
if respond_to?(:"#{key}=") && !self.class.key?(key)
self.send(:"#{key}=", value)
@@ -282,7 +288,7 @@ def load_from_database(attrs)
end
def ensure_key_exists(name)
- self.class.key(name) unless respond_to?("#{name}=")
+ self.class.key(name) unless respond_to?(:"#{name}=")
end
def set_parent_document(key, value)
@@ -291,21 +297,13 @@ def set_parent_document(key, value)
end
end
- def read_key(key_name)
- if key = keys[key_name.to_s]
- value = key.get(instance_variable_get(:"@#{key_name}"))
- set_parent_document(key, value)
- instance_variable_set(:"@#{key_name}", value)
- end
- end
-
def read_key_before_type_cast(name)
instance_variable_get(:"@#{name}_before_type_cast")
end
def write_key(name, value)
key = keys[name.to_s]
- set_parent_document(key, value)
+ set_parent_document(key, value) if key.embeddable?
instance_variable_set :"@#{name}_before_type_cast", value
instance_variable_set :"@#{name}", key.set(value)
end
View
18 lib/mongo_mapper/plugins/keys/key.rb 100644 → 100755
@@ -5,6 +5,8 @@ module Keys
class Key
attr_accessor :name, :type, :options, :default
+ ID_STR = '_id'
+
def initialize(*args)
options_from_args = args.extract_options!
@name, @type = args.shift.to_s, args.shift
@@ -20,8 +22,14 @@ def ==(other)
end
def embeddable?
- return false unless type.respond_to?(:embeddable?)
- type.embeddable?
+ # This is ugly, but it's fast. We can't use ||= because false is an expected and common value.
@jnunemaker
jnunemaker added a line comment Sep 28, 2012

Makes sense. Basically we are memoizing embeddable. Nothing is going to change so this is legit and I can see how it would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ @embeddable = @embeddable != nil ? @embeddable : begin
+ if type.respond_to?(:embeddable?)
+ type.embeddable?
+ else
+ false
+ end
+ end
end
def number?
@@ -34,9 +42,9 @@ def default?
def get(value)
# Special Case: Generate default _id on access
- value = default_value if name == '_id' && value.nil?
+ value = default_value if value.nil? and name == ID_STR
@jnunemaker
jnunemaker added a line comment Sep 28, 2012

So a nil check is faster than a comparison? I think that makes sense. I can also see the constant, though I prefer IdStr or something like that instead. No biggie.

@cheald
MongoMapper member
cheald added a line comment Sep 28, 2012

It should be, since a nil check should just be a check if the object_id == 4, whereas a comparison has to perform a value check. The big optimization there is avoiding creating a new string to compare against for every invocation though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- if options[:typecast].present?
+ if options[:typecast]
@jnunemaker
jnunemaker added a line comment Sep 28, 2012

🆒

@cheald
MongoMapper member
cheald added a line comment Sep 28, 2012

See the benchmark on #blank? up there - I love the terseness of #present?, but it drops us into Ruby code for comparisons rather than staying in C.

@gaffneyc
gaffneyc added a line comment Sep 28, 2012

if value is not the same as if value.present? since #present? is the equivalent of ! value.blank?. Is it okay if we allow empty strings here?

@cheald
MongoMapper member
cheald added a line comment Sep 28, 2012

We could check if String === options[:typecast] and options[:typecast].length > 0, but it's ~10% slower than just the existential check according to a quick benchmark. That seems pretty overkill-ish for something that'd be a pretty extreme edge case. I'd be genuinely surprised it someone is defining a key with a blank typecast.

@jnunemaker
jnunemaker added a line comment Sep 28, 2012

@gaffneyc it is the same intent in this instance. :typecast should always be a class or nil. If you use an empty string you deserve to get a crappy error message. :)

@gaffneyc
gaffneyc added a line comment Sep 28, 2012

@jnunemaker haha good point 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
type.from_mongo(value).map! { |v| typecast_class.from_mongo(v) }
else
type.from_mongo(value)
@@ -45,7 +53,7 @@ def get(value)
def set(value)
type.to_mongo(value).tap do |values|
- if options[:typecast].present?
+ if options[:typecast]
values.map! { |v| typecast_class.to_mongo(v) }
end
end