Skip to content

Hash / HashWithIndifferentAccess speed improvements#35771

Merged
rafaelfranca merged 4 commits into
rails:masterfrom
timoschilling:hash-speed-improvements
Apr 2, 2019
Merged

Hash / HashWithIndifferentAccess speed improvements#35771
rafaelfranca merged 4 commits into
rails:masterfrom
timoschilling:hash-speed-improvements

Conversation

@timoschilling

@timoschilling timoschilling commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

Summary

This changes improves the speed of Hash#except, HashWithIndifferentAccess#except, HashWithIndifferentAccess#values_at and HashWithIndifferentAccess#fetch_values by using ruby native methods.

Benchmarks

Warming up --------------------------------------
     Hash new except   144.256k i/100ms
     Hash old except   104.493k i/100ms
Calculating -------------------------------------
     Hash new except      2.380M (± 1.6%) i/s -     11.973M in   5.032136s
     Hash old except      1.475M (± 2.1%) i/s -      7.419M in   5.031036s

Comparison:
     Hash new except:  2379993.2 i/s
     Hash old except:  1475361.3 i/s - 1.61x  slower
Warming up --------------------------------------
     HWIA new except    35.468k i/100ms
     HWIA old except    27.393k i/100ms
Calculating -------------------------------------
     HWIA new except    381.621k (± 3.1%) i/s -      1.915M in   5.024013s
     HWIA old except    298.944k (± 2.1%) i/s -      1.507M in   5.042050s

Comparison:
     HWIA new except:   381620.8 i/s
     HWIA old except:   298943.8 i/s - 1.28x  slower
Warming up --------------------------------------
   HWIA new value_at   105.698k i/100ms
   HWIA old value_at    85.136k i/100ms
Calculating -------------------------------------
   HWIA new value_at      1.448M (± 1.4%) i/s -      7.293M in   5.036060s
   HWIA old value_at      1.102M (± 1.7%) i/s -      5.534M in   5.023380s

Comparison:
   HWIA new value_at:  1448464.1 i/s
   HWIA old value_at:  1101935.4 i/s - 1.31x  slower
Warming up --------------------------------------
HWIA new fetch_values
                        95.077k i/100ms
HWIA old fetch_values
                        87.582k i/100ms
Calculating -------------------------------------
HWIA new fetch_values
                          1.446M (± 3.6%) i/s -      7.226M in   5.003683s
HWIA old fetch_values
                          1.070M (±13.6%) i/s -      5.255M in   5.025374s

Comparison:
HWIA new fetch_values:  1446069.4 i/s
HWIA old fetch_values:  1070392.3 i/s - 1.35x  slower

Source

Hash#except
New total allocated: 384 bytes (4 objects)
Old total allocated: 312 bytes (3 objects)

HashWithIndifferentAccess#except
New total allocated: 1240 bytes (11 objects)
Old total allocated: 1712 bytes (14 objects)

HashWithIndifferentAccess#values_at
New total allocated: 200 bytes (5 objects)
Old total allocated: 160 bytes (4 objects)

HashWithIndifferentAccess#fetch_values
New total allocated: 200 bytes (5 objects)
Old total allocated: 240 bytes (6 objects)

@rafaelfranca

Copy link
Copy Markdown
Member

Thank you for the pull request. For future reference, can you show the numbers on how fast this is comparing with the previous implementation?

@kaspth

kaspth commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

Some memory profiling would be interesting to see too

@timoschilling

Copy link
Copy Markdown
Contributor Author

@rafaelfranca I updated the summary
@kaspth Can you guide me with that?

@timoschilling timoschilling force-pushed the hash-speed-improvements branch from 1871542 to 90e9004 Compare March 27, 2019 20:59
@kaspth

kaspth commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

https://github.com/SamSaffron/memory_profiler is one way to generate those reports. Especially this new version seems like it generates lots of intermediate objects.

@timoschilling

Copy link
Copy Markdown
Contributor Author

@kaspth Do you think that because of convert_key? I assume that didn't make a difference because of the old implementation uses delete which uses convert_key too.

@kaspth

kaspth commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

I'm thinking about memory because slice, self.keys, -, keys and map all create intermediate arrays.

@timoschilling

Copy link
Copy Markdown
Contributor Author

I added the profile results based on the test methods from the benchmark.

@rafaelfranca

Copy link
Copy Markdown
Member

HashWithIndifferentAccess tests seems to be broken.

@timoschilling

Copy link
Copy Markdown
Contributor Author

I will take a look on that in the next days

@timoschilling timoschilling force-pushed the hash-speed-improvements branch from 90e9004 to 4bffa41 Compare March 28, 2019 23:07
@timoschilling timoschilling force-pushed the hash-speed-improvements branch from 4bffa41 to 9a2bbd0 Compare March 29, 2019 15:29
@timoschilling timoschilling force-pushed the hash-speed-improvements branch from 9a2bbd0 to a805d72 Compare April 1, 2019 15:15
@timoschilling

timoschilling commented Apr 2, 2019

Copy link
Copy Markdown
Contributor Author

I fixed the specs and add a optimisation for HashWithIndifferentAccess#values_at and HashWithIndifferentAccess#fetch_values too.

@timoschilling timoschilling force-pushed the hash-speed-improvements branch 4 times, most recently from 491401e to 3fbad17 Compare April 2, 2019 15:19

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be safe to call map! here, due to the splat on indices (i.e. it will be a newly-allocated array).

@timoschilling timoschilling Apr 2, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes you can safe one object allocation, but map! needs significant more time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What makes you say that? On my machine map! is consistently 10% faster than map.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as previous comment:

It should be safe to call map! here, due to the splat on keys.

@timoschilling timoschilling force-pushed the hash-speed-improvements branch from 3fbad17 to 9f41edb Compare April 2, 2019 20:45
@rafaelfranca rafaelfranca merged commit 736c7d5 into rails:master Apr 2, 2019
@timoschilling timoschilling deleted the hash-speed-improvements branch April 3, 2019 17:17
koic added a commit to koic/thor that referenced this pull request Jul 30, 2020
This PR supports `Thor::CoreExt::HashWithIndifferentAccess#except`
and prevents breaking changes in Rails upgrades when using
`option.except(:key)` in Thor task.

When Thor is used with Rails (Active Support), the behavior changes as follows.

## With Rails 5.2 or lower

```ruby
h = Thor::CoreExt::HashWithIndifferentAccess.new(foo: 1, bar: 2)
h.except(:foo) #=> {"bar"=>2}
```

## With Rails 6.0

```ruby
h = Thor::CoreExt::HashWithIndifferentAccess.new(foo: 1, bar: 2)
h.except(:foo) #=> {"foo"=>1, "bar"=>2}
```

This difference behavior is due to the following changes in Rails 6.0.
rails/rails#35771

This PR makes the behavior between Rails 5.2 and Rails 6.0 compatible.
koic added a commit to koic/thor that referenced this pull request Jul 30, 2020
This PR supports `Thor::CoreExt::HashWithIndifferentAccess#except`
and prevents breaking changes in Rails upgrades when using
`options.except(:key)` in Thor task.

When Thor is used with Rails (Active Support), the behavior changes as follows.

## With Rails 5.2 or lower

```ruby
h = Thor::CoreExt::HashWithIndifferentAccess.new(foo: 1, bar: 2)
h.except(:foo) #=> {"bar"=>2}
```

## With Rails 6.0

```ruby
h = Thor::CoreExt::HashWithIndifferentAccess.new(foo: 1, bar: 2)
h.except(:foo) #=> {"foo"=>1, "bar"=>2}
```

This difference behavior is due to the following changes in Rails 6.0.
rails/rails#35771

This PR makes the behavior between Rails 5.2 and Rails 6.0 compatible.
koic added a commit to koic/thor that referenced this pull request Jul 30, 2020
This PR supports `Thor::CoreExt::HashWithIndifferentAccess#except`
and prevents breaking changes in Rails upgrades when using
`options.except(:key)` in Thor task.

When Thor is used with Rails (Active Support), the behavior changes as follows.

## With Rails 5.2 or lower

```ruby
h = Thor::CoreExt::HashWithIndifferentAccess.new(foo: 1, bar: 2)
h.except(:foo) #=> {"bar"=>2}
```

## With Rails 6.0

```ruby
h = Thor::CoreExt::HashWithIndifferentAccess.new(foo: 1, bar: 2)
h.except(:foo) #=> {"foo"=>1, "bar"=>2}
```

This difference behavior is due to the following changes in Rails 6.0.
rails/rails#35771

This PR makes the behavior between Rails 5.2 and Rails 6.0 compatible.
purecodecoach added a commit to purecodecoach/thor that referenced this pull request Jan 13, 2023
This PR supports `Thor::CoreExt::HashWithIndifferentAccess#except`
and prevents breaking changes in Rails upgrades when using
`options.except(:key)` in Thor task.

When Thor is used with Rails (Active Support), the behavior changes as follows.

## With Rails 5.2 or lower

```ruby
h = Thor::CoreExt::HashWithIndifferentAccess.new(foo: 1, bar: 2)
h.except(:foo) #=> {"bar"=>2}
```

## With Rails 6.0

```ruby
h = Thor::CoreExt::HashWithIndifferentAccess.new(foo: 1, bar: 2)
h.except(:foo) #=> {"foo"=>1, "bar"=>2}
```

This difference behavior is due to the following changes in Rails 6.0.
rails/rails#35771

This PR makes the behavior between Rails 5.2 and Rails 6.0 compatible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants