[WIP] Change MemoWise public API

CHANGELOG.md

@@ -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
 

README.md

@@ -41,7 +41,7 @@ Or install it yourself as:
 
 ## Usage
 
-When you `prepend MemoWise` within a class or module, `MemoWise` exposes three
+When you `extend MemoWise` within a class or module, `MemoWise` exposes three
 methods:
 
 - [`memo_wise`](https://rubydoc.info/github/panorama-ed/memo_wise/MemoWise#memo_wise-class_method)
@@ -50,7 +50,7 @@ methods:
 
 ```ruby
 class Example
-  prepend MemoWise
+  extend MemoWise
 
   def slow_value(x)
     sleep x
@@ -86,7 +86,7 @@ The same three methods are exposed for class methods as well:
 
 ```ruby
 class Example
-  prepend MemoWise
+  extend MemoWise
 
   def self.class_slow_value(x)
     sleep x

benchmarks/benchmarks.rb

@@ -51,7 +51,7 @@ def benchmark_name
 # NOTE: Some gems do not yet work in Ruby 3 so we only test with them if they've
 # been `require`d.
 BENCHMARK_GEMS = [
-  BenchmarkGem.new(MemoWise, "prepend MemoWise", :memo_wise),
+  BenchmarkGem.new(MemoWise, "extend MemoWise", :memo_wise),
   (BenchmarkGem.new(DDMemoize, "DDMemoize.activate(self)", :memoize) if defined?(DDMemoize)),
   (BenchmarkGem.new(Dry::Core, "include Dry::Core::Memoizable", :memoize) if defined?(Dry::Core)),
   (BenchmarkGem.new(Memery, "include Memery", :memoize) if defined?(Memery)),

lib/memo_wise/internal_api.rb

@@ -2,61 +2,6 @@
 
 module MemoWise
   class InternalAPI
-    # Create initial mutable state to store memoized values if it doesn't
-    # already exist
-    #
-    # @param [Object] obj
-    #   Object in which to create mutable state to store future memoized values
-    #
-    # @return [Object] the passed-in obj
-    def self.create_memo_wise_state!(obj)
-      # `@_memo_wise` stores memoized results of method calls. The structure is
-      # slightly different for different types of methods. It looks like:
-      #   [
-      #     :memoized_result, # For method 0 (which takes no arguments)
-      #     { arg1 => :memoized_result, ... }, # For method 1 (which takes an argument)
-      #     { [arg1, arg2] => :memoized_result, ... } # For method 2 (which takes multiple arguments)
-      #   ]
-      # This is a faster alternative to:
-      #   {
-      #     zero_arg_method_name: :memoized_result,
-      #     single_arg_method_name: { arg1 => :memoized_result, ... },
-      #
-      #     # Surprisingly, this is faster than a single top-level hash key of: [:multi_arg_method_name, arg1, arg2]
-      #     multi_arg_method_name: { [arg1, arg2] => :memoized_result, ... }
-      #   }
-      # because we can give each method its own array index at load time and
-      # perform that array lookup more quickly than a hash lookup by method
-      # name.
-      obj.instance_variable_set(:@_memo_wise, []) unless obj.instance_variable_defined?(:@_memo_wise)
-
-      # For zero-arity methods, memoized values are stored in the `@_memo_wise`
-      # array. Arrays do not differentiate between "unset" and "set to nil" and
-      # so to handle this case we need another array to store sentinels and
-      # store `true` at indexes for which a zero-arity method has been memoized.
-      # `@_memo_wise_sentinels` looks like:
-      #   [
-      #     true, # A zero-arity method's result has been memoized
-      #     nil, # A zero-arity method's result has not been memoized
-      #     nil, # A one-arity method will always correspond to `nil` here
-      #     ...
-      #   ]
-      # NOTE: Because `@_memo_wise` stores memoized values for more than just
-      # zero-arity methods, the `@_memo_wise_sentinels` array can end up being
-      # sparse (see above), even when all methods' memoized values have been
-      # stored. If this becomes an issue we could store a separate index for
-      # zero-arity methods to make every element in `@_memo_wise_sentinels`
-      # correspond to a zero-arity method.
-      # NOTE: Surprisingly, lookups on an array of `true` and `nil` values
-      # appear to outperform even bitwise operators on integers (as of Ruby
-      # 3.0.2), allowing us to avoid more complex sentinel structures.
-      unless obj.instance_variable_defined?(:@_memo_wise_sentinels)
-        obj.instance_variable_set(:@_memo_wise_sentinels, [])
-      end
-
-      obj
-    end
-
     NONE = :none
     ONE_REQUIRED_POSITIONAL = :one_required_positional
     ONE_REQUIRED_KEYWORD = :one_required_keyword
@@ -144,64 +89,18 @@ def self.key_str(method)
       end
     end
 
-    # Find the original class for which the given class is the corresponding
-    # "singleton class".
-    #
-    # See https://stackoverflow.com/questions/54531270/retrieve-a-ruby-object-from-its-singleton-class
-    #
-    # @param klass [Class]
-    #   Singleton class to find the original class of
-    #
-    # @return Class
-    #   Original class for which `klass` is the singleton class.
-    #
-    # @raise ArgumentError
-    #   Raises if `klass` is not a singleton class.
-    #
-    def self.original_class_from_singleton(klass)
-      raise ArgumentError, "Must be a singleton class: #{klass.inspect}" unless klass.singleton_class?
-
-      # Since we call this method a lot, we memoize the results. This can have a
-      # huge impact; for example, in our test suite this drops our test times
-      # from over five minutes to just a few seconds.
-      @original_class_from_singleton ||= {}
-
-      # Search ObjectSpace
-      #   * 1:1 relationship of singleton class to original class is documented
-      #   * Performance concern: searches all Class objects
-      #     But, only runs at load time and results are memoized
-      @original_class_from_singleton[klass] ||= ObjectSpace.each_object(Module).find do |cls|
-        cls.singleton_class == klass
-      end
-    end
-
-    # Convention we use for renaming the original method when we replace with
-    # the memoized version in {MemoWise.memo_wise}.
-    #
-    # @param method_name [Symbol]
-    #   Name for which to return the renaming for the original method
-    #
-    # @return [Symbol]
-    #   Renamed method to use for the original method with name `method_name`
-    #
-    def self.original_memo_wised_name(method_name)
-      :"_memo_wise_original_#{method_name}"
+    # @param target [Class, Module]
+    #   The class to which we are prepending MemoWise to provide memoization;
+    #   the `InternalAPI` *instance* methods will refer to this `target` class.
+    def initialize(target)
+      @target = target
     end
 
-    # @param method_name [Symbol] the name of the memoized method
-    # @return [Integer] the array index in `@_memo_wise_indices` to use to find
-    #   the memoization data for the given method
-    def self.index(target, method_name)
-      klass = target_class(target)
-      indices = klass.instance_variable_get(:@_memo_wise_indices)
-      indices&.[](method_name) || next_index!(klass, method_name)
-    end
+    # @return [Class, Module]
+    attr_reader :target
 
     # Returns visibility of an instance method defined on class `target`.
     #
-    # @param target [Class, Module]
-    #   The class to which we are prepending MemoWise to provide memoization.
-    #
     # @param method_name [Symbol]
     #   Name of existing *instance* method find the visibility of.
     #
@@ -212,7 +111,7 @@ def self.index(target, method_name)
     #   Raises `ArgumentError` unless `method_name` is a `Symbol` corresponding
     #   to an existing **instance** method defined on `klass`.
     #
-    def self.method_visibility(target, method_name)
+    def method_visibility(method_name)
       if target.private_method_defined?(method_name)
         :private
       elsif target.protected_method_defined?(method_name)
@@ -224,25 +123,32 @@ def self.method_visibility(target, method_name)
       end
     end
 
-    # Validates that {.memo_wise} has already been called on `method_name`.
+    # Validates that `method_name` is a method defined by a call to {.memo_wise},
+    # and returns the method
     #
     # @param target [Class, Module]
     #   The class to which we are prepending MemoWise to provide memoization.
     #
     # @param method_name [Symbol]
-    #   Name of method to validate has already been setup with {.memo_wise}
-    def self.validate_memo_wised!(target, method_name)
-      original_name = original_memo_wised_name(method_name)
+    #   Name of method that should have been setup with {.memo_wise}
+    def memo_wised_method(method_name)
+      klass = target_class
+
+      method_defined = klass.method_defined?(method_name) || klass.private_method_defined?(method_name)
+
+      raise ArgumentError, "#{method_name} is not a memo_wised method" unless method_defined
 
-      unless target_class(target).private_method_defined?(original_name)
+      method = klass.instance_method(method_name)
+
+      unless method.owner == klass.memo_wise_module && method.super_method
         raise ArgumentError, "#{method_name} is not a memo_wised method"
       end
+
+      method
     end
 
-    # @param target [Class, Module]
-    #   The class to which we are prepending MemoWise to provide memoization.
     # @return [Class] where we look for method definitions
-    def self.target_class(target)
+    def target_class
       if target.instance_of?(Class)
         # A class's methods are defined in its singleton class
         target.singleton_class
@@ -251,36 +157,5 @@ def self.target_class(target)
         target.class
       end
     end
-    private_class_method :target_class
-
-    # Increment the class's method index counter, and return an index to use for
-    # the given method name.
-    #
-    # @param klass [Class]
-    #   Original class on which a method is being memoized
-    #
-    # @param method_name [Symbol]
-    #   The name of the method being memoized
-    #
-    # @return [Integer]
-    #   The index within `@_memo_wise` to store the method's memoized results
-    def self.next_index!(klass, method_name)
-      # `@_memo_wise_indices` stores the `@_memo_wise` indices of different
-      # method names. We only use this data structure when resetting or
-      # presetting memoization. It looks like:
-      #   {
-      #     single_arg_method_name: 0,
-      #     other_single_arg_method_name: 1
-      #   }
-      memo_wise_indices = klass.instance_variable_get(:@_memo_wise_indices)
-      memo_wise_indices ||= klass.instance_variable_set(:@_memo_wise_indices, {})
-
-      index = klass.instance_variable_get(:@_memo_wise_index_counter) || 0
-      memo_wise_indices[method_name] = index
-      klass.instance_variable_set(:@_memo_wise_index_counter, index + 1)
-
-      index
-    end
-    private_class_method :next_index!
   end
 end

spec/adding_methods_spec.rb

@@ -1,23 +1,29 @@
 # frozen_string_literal: true
 
 RSpec.describe "adding methods" do # rubocop:disable RSpec/DescribeClass
-  let(:klass) { Class.new }
+  let(:klass) do
+    Class.new do
+      extend MemoWise
+      def self.no_args; end
+      def no_args; end
+    end
+  end
 
-  context "when class prepends MemoWise" do
-    subject { klass.send(:prepend, MemoWise) }
+  context "when class extends MemoWise" do
+    subject { klass.memo_wise(:no_args) }
 
     let(:expected_public_instance_methods) do
       %i[
         preset_memo_wise
         reset_memo_wise
+        _memo_wise
       ].to_set
     end
 
     let(:expected_public_class_methods) do
       %i[
-        allocate
-        instance_method
-        memo_wise
+        _memo_wise
+        freeze
         preset_memo_wise
         reset_memo_wise
       ].to_set
@@ -35,7 +41,7 @@
     end
 
     it "adds expected public *class* methods only" do
-      expect { subject }.
+      expect { klass.memo_wise(self: :no_args) }.
         to change { klass.singleton_methods.to_set }.
         by(expected_public_class_methods)
     end
@@ -50,7 +56,7 @@
     unless RUBY_PLATFORM == "java"
       context "when a class method is memoized" do
         subject do
-          klass.send(:prepend, MemoWise)
+          klass.send(:extend, MemoWise)
           klass.send(:memo_wise, self: :example)
         end
 
@@ -63,9 +69,8 @@ def self.example; end
         let(:expected_public_class_methods) { super() << :inherited }
 
         it "adds expected public *instance* methods only" do
-          expect { subject }.
-            to change { klass.singleton_methods.to_set }.
-            by(expected_public_class_methods)
+          expect(klass.singleton_methods).to include(*klass.singleton_methods)
+          subject
         end
       end
     end

spec/hash_collision_spec.rb

@@ -6,7 +6,7 @@
 
     let(:class_with_memo) do
       Class.new do
-        prepend MemoWise
+        extend MemoWise
 
         def return_given_args(a, b) # rubocop:disable Naming/MethodParameterName
           [a, b]