Skip to content

Commit

Permalink
Merge pull request #250 from panorama-ed/back-to-the-future
Browse files Browse the repository at this point in the history 
Revert internal data structure to simple hash
  • Loading branch information
@JacobEvelyn
JacobEvelyn authored Dec 15, 2021
2 parents 5581c30 + db25003 commit a2f87b0
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 267 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

- Nothing yet!
- Switch to a simpler internal data structure to fix several classes of bugs
that the previous few versions were unable to sufficiently address

## [1.4.0] - 2021-12-10

Expand Down
36 changes: 18 additions & 18 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,15 @@ Results using Ruby 3.0.3:

|Method arguments|`Dry::Core`\* (0.7.1)|`Memery` (1.4.0)|
|--|--|--|
|`()` (none)|1.36x|19.42x|
|`(a)`|2.47x|11.39x|
|`(a, b)`|0.44x|2.16x|
|`(a:)`|2.30x|22.89x|
|`(a:, b:)`|0.47x|4.54x|
|`(a, b:)`|0.47x|4.33x|
|`(a, *args)`|0.83x|2.09x|
|`(a:, **kwargs)`|0.76x|2.85x|
|`(a, *args, b:, **kwargs)`|0.61x|1.55x|
|`()` (none)|1.29x|17.21x|
|`(a)`|2.33x|11.03x|
|`(a, b)`|0.44x|2.00x|
|`(a:)`|2.06x|21.20x|
|`(a:, b:)`|0.47x|4.40x|
|`(a, b:)`|0.47x|4.27x|
|`(a, *args)`|0.86x|1.95x|
|`(a:, **kwargs)`|0.77x|3.01x|
|`(a, *args, b:, **kwargs)`|0.59x|1.60x|

\* `Dry::Core`
[may cause incorrect behavior caused by hash collisions](https://github.com/dry-rb/dry-core/issues/63).
Expand All @@ -135,15 +135,15 @@ Results using Ruby 2.7.5 (because these gems raise errors in Ruby 3.x):

|Method arguments|`DDMemoize` (1.0.0)|`Memoist` (0.16.2)|`Memoized` (1.0.2)|`Memoizer` (1.0.3)|
|--|--|--|--|--|
|`()` (none)|36.84x|3.56x|1.68x|4.19x|
|`(a)`|27.50x|18.85x|13.97x|15.99x|
|`(a, b)`|3.27x|2.34x|1.85x|2.05x|
|`(a:)`|37.22x|30.09x|25.57x|27.28x|
|`(a:, b:)`|5.25x|4.38x|3.80x|4.02x|
|`(a, b:)`|5.08x|4.15x|3.56x|3.78x|
|`(a, *args)`|3.17x|2.32x|1.96x|2.01x|
|`(a:, **kwargs)`|2.87x|2.42x|2.10x|2.21x|
|`(a, *args, b:, **kwargs)`|2.05x|1.76x|1.63x|1.65x|
|`()` (none)|31.13x|3.12x|1.51x|3.57x|
|`(a)`|23.95x|16.52x|12.11x|14.00x|
|`(a, b)`|3.18x|2.31x|1.82x|2.00x|
|`(a:)`|33.55x|27.09x|23.21x|24.68x|
|`(a:, b:)`|5.20x|4.28x|3.82x|3.99x|
|`(a, b:)`|4.96x|4.09x|3.57x|3.76x|
|`(a, *args)`|3.10x|2.32x|1.95x|1.97x|
|`(a:, **kwargs)`|2.89x|2.43x|2.12x|2.25x|
|`(a, *args, b:, **kwargs)`|2.05x|1.77x|1.63x|1.65x|

You can run benchmarks yourself with:

Expand Down
16 changes: 2 additions & 14 deletions benchmarks/benchmarks.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,6 @@ def no_args
end
#{benchmark_gem.memoization_method} :no_args
# For the no_args case, we can't depend on arguments to alternate between
# returning truthy and falsey values, so instead make two separate
# no_args methods
def no_args_falsey
nil
end
#{benchmark_gem.memoization_method} :no_args_falsey
def one_positional_arg(a)
100 if a.positive?
end
Expand Down Expand Up @@ -132,9 +124,7 @@ def positional_splat_keyword_and_double_splat_args(a, *args, b:, **kwargs)
#
# NOTE: The proportion of falsey results is 1/N_UNIQUE_ARGUMENTS (because for
# the methods with arguments we are truthy for all but the first unique argument
# set, and for zero-arity methods we manually execute `no_args` N_TRUTHY_RESULTS
# times per each execution of `no_args_falsey`). This number was selected as the
# lowest number such that this logic:
# set). This number was selected as the lowest number such that this logic:
#
# output = hash[key]
# if output || hash.key?(key)
Expand Down Expand Up @@ -165,12 +155,10 @@ def positional_splat_keyword_and_double_splat_args(a, *args, b:, **kwargs)
# result value, so our benchmark only tests memoized retrieval time.
benchmark_lambdas = [
lambda do |x, instance, benchmark_gem|
instance.no_args_falsey
instance.no_args

x.report("#{benchmark_gem.benchmark_name}: ()") do
instance.no_args_falsey
N_TRUTHY_RESULTS.times { instance.no_args }
instance.no_args
end
end,
lambda do |x, instance, benchmark_gem|
Expand Down
198 changes: 45 additions & 153 deletions lib/memo_wise.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,72 +180,26 @@ def inherited(subclass)

case method_arguments
when MemoWise::InternalAPI::NONE
# Zero-arg methods can use simpler/more performant logic because the
# hash key is just the method name.
klass.send(:define_method, method_name) do # Ruby 2.4's `define_method` is private in some cases
index = MemoWise::InternalAPI.index(self, method_name)
klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1
def #{method_name}
if @_memo_wise_sentinels[#{index}]
@_memo_wise[#{index}]
else
ret = @_memo_wise[#{index}] = #{original_memo_wised_name}
@_memo_wise_sentinels[#{index}] = true
ret
end
klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1
def #{method_name}
@_memo_wise.fetch(:#{method_name}) do
@_memo_wise[:#{method_name}] = #{original_memo_wised_name}
end
HEREDOC

klass.send(visibility, method_name)
send(method_name)
end
end
HEREDOC
when MemoWise::InternalAPI::ONE_REQUIRED_POSITIONAL, MemoWise::InternalAPI::ONE_REQUIRED_KEYWORD
key = method.parameters.first.last
# NOTE: Ruby 2.6 and below, and TruffleRuby 3.0, break when we use
# `define_method(...) do |*args, **kwargs|`. Instead we must use the
# simpler `|*args|` pattern. We can't just do this always though
# because Ruby 2.7 and above require `|*args, **kwargs|` to work
# correctly.
# See: https://blog.saeloun.com/2019/10/07/ruby-2-7-keyword-arguments-redesign.html#ruby-26
# :nocov:
if RUBY_VERSION < "2.7" || RUBY_ENGINE == "truffleruby"
klass.send(:define_method, method_name) do |*args| # Ruby 2.4's `define_method` is private in some cases
index = MemoWise::InternalAPI.index(self, method_name)
klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1
def #{method_name}(#{MemoWise::InternalAPI.args_str(method)})
_memo_wise_hash = (@_memo_wise[#{index}] ||= {})
_memo_wise_output = _memo_wise_hash[#{key}]
if _memo_wise_output || _memo_wise_hash.key?(#{key})
_memo_wise_output
else
_memo_wise_hash[#{key}] = #{original_memo_wised_name}(#{MemoWise::InternalAPI.call_str(method)})
end
end
HEREDOC

klass.send(visibility, method_name)
send(method_name, *args)
end
# :nocov:
else
klass.define_method(method_name) do |*args, **kwargs|
index = MemoWise::InternalAPI.index(self, method_name)
klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1
def #{method_name}(#{MemoWise::InternalAPI.args_str(method)})
_memo_wise_hash = (@_memo_wise[#{index}] ||= {})
_memo_wise_output = _memo_wise_hash[#{key}]
if _memo_wise_output || _memo_wise_hash.key?(#{key})
_memo_wise_output
else
_memo_wise_hash[#{key}] = #{original_memo_wised_name}(#{MemoWise::InternalAPI.call_str(method)})
end
end
HEREDOC

klass.send(visibility, method_name)
send(method_name, *args, **kwargs)
klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1
def #{method_name}(#{MemoWise::InternalAPI.args_str(method)})
_memo_wise_hash = (@_memo_wise[:#{method_name}] ||= {})
_memo_wise_output = _memo_wise_hash[#{key}]
if _memo_wise_output || _memo_wise_hash.key?(#{key})
_memo_wise_output
else
_memo_wise_hash[#{key}] = #{original_memo_wised_name}(#{MemoWise::InternalAPI.call_str(method)})
end
end
end
HEREDOC
# MemoWise::InternalAPI::MULTIPLE_REQUIRED, MemoWise::InternalAPI::SPLAT,
# MemoWise::InternalAPI::DOUBLE_SPLAT, MemoWise::InternalAPI::SPLAT_AND_DOUBLE_SPLAT
else
Expand All @@ -261,54 +215,18 @@ def #{method_name}(#{MemoWise::InternalAPI.args_str(method)})
# consistent performance. In general, this should still be faster for
# truthy results because `Hash#[]` generally performs hash lookups
# faster than `Hash#fetch`.
#
# NOTE: Ruby 2.6 and below, and TruffleRuby 3.0, break when we use
# `define_method(...) do |*args, **kwargs|`. Instead we must use the
# simpler `|*args|` pattern. We can't just do this always though
# because Ruby 2.7 and above require `|*args, **kwargs|` to work
# correctly.
# See: https://blog.saeloun.com/2019/10/07/ruby-2-7-keyword-arguments-redesign.html#ruby-26
# :nocov:
if RUBY_VERSION < "2.7" || RUBY_ENGINE == "truffleruby"
klass.send(:define_method, method_name) do |*args| # Ruby 2.4's `define_method` is private in some cases
index = MemoWise::InternalAPI.index(self, method_name)
klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1
def #{method_name}(#{MemoWise::InternalAPI.args_str(method)})
_memo_wise_hash = (@_memo_wise[#{index}] ||= {})
_memo_wise_key = #{MemoWise::InternalAPI.key_str(method)}
_memo_wise_output = _memo_wise_hash[_memo_wise_key]
if _memo_wise_output || _memo_wise_hash.key?(_memo_wise_key)
_memo_wise_output
else
_memo_wise_hash[_memo_wise_key] = #{original_memo_wised_name}(#{MemoWise::InternalAPI.call_str(method)})
end
end
HEREDOC

klass.send(visibility, method_name)
send(method_name, *args)
end
# :nocov:
else # Ruby 2.7 and above break with (*args)
klass.define_method(method_name) do |*args, **kwargs|
index = MemoWise::InternalAPI.index(self, method_name)
klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1
def #{method_name}(#{MemoWise::InternalAPI.args_str(method)})
_memo_wise_hash = (@_memo_wise[#{index}] ||= {})
_memo_wise_key = #{MemoWise::InternalAPI.key_str(method)}
_memo_wise_output = _memo_wise_hash[_memo_wise_key]
if _memo_wise_output || _memo_wise_hash.key?(_memo_wise_key)
_memo_wise_output
else
_memo_wise_hash[_memo_wise_key] = #{original_memo_wised_name}(#{MemoWise::InternalAPI.call_str(method)})
end
end
HEREDOC

klass.send(visibility, method_name)
send(method_name, *args, **kwargs)
klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1
def #{method_name}(#{MemoWise::InternalAPI.args_str(method)})
_memo_wise_hash = (@_memo_wise[:#{method_name}] ||= {})
_memo_wise_key = #{MemoWise::InternalAPI.key_str(method)}
_memo_wise_output = _memo_wise_hash[_memo_wise_key]
if _memo_wise_output || _memo_wise_hash.key?(_memo_wise_key)
_memo_wise_output
else
_memo_wise_hash[_memo_wise_key] = #{original_memo_wised_name}(#{MemoWise::InternalAPI.call_str(method)})
end
end
end
HEREDOC
end

klass.send(visibility, method_name)
Expand Down Expand Up @@ -511,21 +429,20 @@ def target.instance_method(symbol)
# ex.method_called_times #=> nil
#
def preset_memo_wise(method_name, *args, **kwargs)
raise ArgumentError, "#{method_name.inspect} must be a Symbol" unless method_name.is_a?(Symbol)
raise ArgumentError, "Pass a block as the value to preset for #{method_name}, #{args}" unless block_given?

MemoWise::InternalAPI.validate_memo_wised!(self, method_name)

method = method(MemoWise::InternalAPI.original_memo_wised_name(method_name))
method_arguments = MemoWise::InternalAPI.method_arguments(method)
index = MemoWise::InternalAPI.index(self, method_name)

if method_arguments == MemoWise::InternalAPI::NONE
@_memo_wise_sentinels[index] = true
@_memo_wise[index] = yield
@_memo_wise[method_name] = yield
return
end

hash = (@_memo_wise[index] ||= {})
hash = (@_memo_wise[method_name] ||= {})

case method_arguments
when MemoWise::InternalAPI::ONE_REQUIRED_POSITIONAL then hash[args.first] = yield
Expand Down Expand Up @@ -613,7 +530,6 @@ def reset_memo_wise(method_name = nil, *args, **kwargs)
raise ArgumentError, "Provided kwargs when method_name = nil" unless kwargs.empty?

@_memo_wise.clear
@_memo_wise_sentinels.clear
return
end

Expand All @@ -624,49 +540,25 @@ def reset_memo_wise(method_name = nil, *args, **kwargs)

method = method(MemoWise::InternalAPI.original_memo_wised_name(method_name))
method_arguments = MemoWise::InternalAPI.method_arguments(method)
index = MemoWise::InternalAPI.index(self, method_name)

# method_name == MemoWise::InternalAPI::NONE will be covered by this case.
@_memo_wise.delete(method_name) if args.empty? && kwargs.empty?
method_hash = @_memo_wise[method_name]

case method_arguments
when MemoWise::InternalAPI::NONE
@_memo_wise_sentinels[index] = nil
@_memo_wise[index] = nil
when MemoWise::InternalAPI::ONE_REQUIRED_POSITIONAL
if args.empty?
@_memo_wise[index]&.clear
else
@_memo_wise[index]&.delete(args.first)
end
when MemoWise::InternalAPI::ONE_REQUIRED_KEYWORD
if kwargs.empty?
@_memo_wise[index]&.clear
else
@_memo_wise[index]&.delete(kwargs.first.last)
end
when MemoWise::InternalAPI::SPLAT
if args.empty?
@_memo_wise[index]&.clear
else
@_memo_wise[index]&.delete(args)
end
when MemoWise::InternalAPI::DOUBLE_SPLAT
if kwargs.empty?
@_memo_wise[index]&.clear
else
@_memo_wise[index]&.delete(kwargs)
end
when MemoWise::InternalAPI::ONE_REQUIRED_POSITIONAL then method_hash&.delete(args.first)
when MemoWise::InternalAPI::ONE_REQUIRED_KEYWORD then method_hash&.delete(kwargs.first.last)
when MemoWise::InternalAPI::SPLAT then method_hash&.delete(args)
when MemoWise::InternalAPI::DOUBLE_SPLAT then method_hash&.delete(kwargs)
else # MemoWise::InternalAPI::MULTIPLE_REQUIRED, MemoWise::InternalAPI::SPLAT_AND_DOUBLE_SPLAT
if args.empty? && kwargs.empty?
@_memo_wise[index]&.clear
else
key = if method_arguments == MemoWise::InternalAPI::SPLAT_AND_DOUBLE_SPLAT
[args, kwargs]
else
method.parameters.map.with_index do |(type, name), i|
type == :req ? args[i] : kwargs[name] # rubocop:disable Metrics/BlockNesting
end
key = if method_arguments == MemoWise::InternalAPI::SPLAT_AND_DOUBLE_SPLAT
[args, kwargs]
else
method.parameters.map.with_index do |(type, name), i|
type == :req ? args[i] : kwargs[name]
end
@_memo_wise[index]&.delete(key)
end
end
method_hash&.delete(key)
end
end
end
Loading

0 comments on commit a2f87b0

Please sign in to comment.