Re: [Rails-core] ActiveSupport Hash#dup

Jeremy Daer

unread,

Apr 29, 2016, 7:52:52 PM4/29/16

to Ruby on Rails: Core

Please do PR. This is a delicate behavior change, so let's take care that the hashes are indeed separate objects, defaults are preserved correctly, etc.

On Tue, Apr 26, 2016 at 2:03 PM Dmitry Gautsel <lvl...@gmail.com> wrote:

Hi all.

What do you think about this fix: Hash#dup => Hash[hash]

For example:

1) Hash#except method:

class Hash

# current behavior
def except(*keys)
dup.except!(*keys)
end

# new behavior
def except2(*keys)
Hash[self].except!(*keys)
end

def except!(*keys)
keys.each { |key| delete(key) }
self
end
end

hash = { a: 123, b: 23, c: 34 }

Benchmark.ips do |x|
x.report('before') { hash.except(:a, :b) }
x.report('after') { hash.except2(:a, :b) }
x.compare!
end

Calculating -------------------------------------
before 41.756k i/100ms
after 62.440k i/100ms
-------------------------------------------------
before 665.171k (± 2.1%) i/s - 3.340M
after 1.116M (± 2.4%) i/s - 5.620M

Comparison:
after: 1115631.6 i/s
before: 665170.7 i/s - 1.68x slower

2) Hash#deep_merge method (Probably not the best example)

class Hash

# current behavior
def deep_merge(other_hash, &block)
dup.deep_merge!(other_hash, &block)
end

# new behavior
def deep_merge2(other_hash, &block)
Hash[self].deep_merge!(other_hash, &block)
end

def deep_merge!(other_hash, &block)
other_hash.each_pair do |current_key, other_value|
this_value = self[current_key]

self[current_key] = if this_value.is_a?(Hash) && other_value.is_a?(Hash)
this_value.deep_merge(other_value, &block)
else
if block_given? && key?(current_key)
block.call(current_key, this_value, other_value)
else
other_value
end
end
end

self
end
end

h1 = { a: true, b: { c: [1, 2, 3] } }
h2 = { a: false, b: { x: [3, 4, 5] } }

h1.deep_merge(h2)

Benchmark.ips do |x|
x.report('before') { h1.deep_merge(h2) }
x.report('after') { h1.deep_merge2(h2) }
x.compare!
end

Calculating -------------------------------------
before 22.259k i/100ms
after 26.394k i/100ms
-------------------------------------------------
before 297.791k (± 2.4%) i/s - 1.491M
after 364.967k (± 1.6%) i/s - 1.848M

Comparison:
after: 364967.2 i/s
before: 297790.9 i/s - 1.23x slower

If you think it's OK, I will fix it.

Regards, Dmitry.

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To post to this group, send email to rubyonra...@googlegroups.com.
Visit this group at https://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.

Dmitry Gautsel

unread,

Apr 30, 2016, 12:55:16 PM4/30/16

to Ruby on Rails: Core

Hi, Jeremy.

It seemed to be a good idea, but then I realized that it is actually not that good.

Because:

h={'test' => 123, ttt: 1234}               # => {"test"=>123, :ttt=>1234}
h=h.with_indifferent_access                # => {"test"=>123, "ttt"=>1234}
h.except(:ttt)                             # => {"test"=>123}
h.except2(:ttt)                            # => {"test"=>123, "ttt"=>1234}

As we can see, except2 method has different behaviour then except method. However I think I can fix it.

class HashWithIndifferentAccess < Hash
  def except2(*keys)
    dup.except!(*keys)
  end
end 
h={'test' => 123, ttt: 1234}               # => {"test"=>123, :ttt=>1234}
h=h.with_indifferent_access                # => {"test"=>123, "ttt"=>1234}
h.except(:ttt)                             # => {"test"=>123}
h.except2(:ttt)                            # => {"test"=>123}

What do you think about it?

Regards, Dmitry.

Jeremy Daer

unread,

Apr 30, 2016, 1:45:10 PM4/30/16

to Ruby on Rails: Core

Good catch. Do we have test coverage for these cases already?

Dmitry Gautsel

unread,

Apr 30, 2016, 2:12:30 PM4/30/16

to Ruby on Rails: Core

I have found "test_deep_merge_on_indifferent_access". However there is no test for except method for these cases.

Reply all

Reply to author

Forward