panorama-ed / memo_wise Goto Github PK

Many of our tests run quite quickly (as they should, since they simply execute pure Ruby), but many are quite slow (as in: >1s per test). The slow ones seem to be within when defined with scope 'class << self' and when defined with scope 'module << self' context blocks, though there may be others I've missed.

Test time currently is a big nuisance when working on features/and speeding up tests would be a big quality-of-life improvement.

For a method like the following:

def foo(arg1, key)
  ...
end

MemoWise will produce a method like the following:

def foo(arg1, key)
  ...
  key = [arg1, key] # Overrides key!
end

Similar bugs are possible for methods with arguments that share names with other variables we use, like output or hash.

We should fix all methods to only use _memo_wise_-prefixed variables.

• Should also support reset_memo_wise and preset_memo_wise on class methods

Note: one approach that might solve this and improve performance overall is using a different instance variable for each method instead of one big hash variable that we instantiate in initialize

This may be due to how GitHub Actions checks out the code

Right now our test ordering is deterministic, which makes it more likely that we'll have hidden dependencies between tests. We should randomize our test ordering instead using RSpec's built-in mechanism for that.

In Panorama spec helper code 'initialized_double', we observed
that MemoWise interrupts the use of Ruby reflection on method
parameters, by replacing with delegating methods that have overly
generic signatures, like:

    def initialize(...)

    def slow_method(*arg, **kwargs)

When we run for example TestClass.instance_method(:initialize).parameters, we get back those generic signatures, rather than the specific parameters that the original methods had.

Your dependency file specified a branch or reference for panolint, but Dependabot couldn't find it at the project's source. Has it been removed?

For Ruby dependencies, this can be caused by a branch specified in your Gemfile being deleted at the source, or having been rebased, so the commit reference in your Gemfile.lock is no longer included in the branch. In that case, it can be fixed by running bundler update panolint locally.

View the update logs.

Perhaps an owl holding a ruby?

Logo should appear at the top of README.md

Use the format as documented in https://github.com/panorama-ed/scan_left/blob/master/CHANGELOG.md

There's a small subset of cases that I think we could optimize even further, depending on how crazy we want to get. If:

• the method we're optimizing has an arity of 0 (no args)
• the result object can be converted to Ruby source code with no loss of information, e.g. it is both frozen and one of: nil, a boolean, a small int (Fixnum?), a symbol, or a string
  • (or maybe an array or hash containing only those elements? what about nested arrays and hashes that are all frozen?)

Then when the method is first called, instead of inserting the value into our method's cache we can actually just rewrite the entire method to hardcode the result. This would make resetting/presetting challenging, and we'd have to be careful about which cases allow this, but I think it could work for some cases.

And since this logic would only occur on the first call, it should overall be a performance win (and in fact we'd only see the upside in our benchmarks). Would love thoughts @jemmaissroff @ms-ati

When working on changes with (potentially) very small performance gains, it's hard to know whether the changes are actually an improvement. Currently, my process is:

1. Change my local branch (with the changes I want to test) to have a new name for MemoWise. This is annoying and requires changing a surprisingly large number of file names, directory names, and lines of code.
2. Change the benchmarks script and Gemfile to load MemoWise from the latest commit on GitHub, in addition to the local changes (which now have a new name).
3. Try to run benchmarks locally but uncover issues with (1) and (2) that I missed.
4. Fix those issues.
5. Run benchmarks locally with as "minimal" of an environment as I can manage.
6. Re-run benchmarks multiple times to confirm results are consistent.

I would love if (for starters) our GitHub Action could somehow compare the current change against the latest commit on main. There's lots of ways we could make this more featureful (e.g. only run this benchmark if a label is set in the PR, and run benchmarks for a longer time than usual for this comparison), but anything that's better than the above would be a huge improvement.

module M1
  prepend MemoWise

  def method_to_memowise ; true ; end
  memo_wise :method_to_memowise
end

module M2
  prepend MemoWise

  def other_method_to_memowise ; false ; end
  memo_wise :other_method_to_memowise
end

class C1
  include M1, M2
end

c = C1.new
puts c.method_to_memowise # Should be true
# => true
puts c.other_method_to_memowise # Should be false
# => true

I'm not sure if there's an elegant way to fix this. The easy solutions I can think of are:

• Use a global index counter instead of one tied to the class hierarchy [downside: objects will have large/sparse arrays storing results]
• Go back to using hashes keyed on method name instead of arrays [downside: hash accesses are slightly slower than array accesses]

Some "more performant" options that may be difficult:

• Have MemoWise delay the module_eval until either (a) the method is called the first time or (b) the object's freeze method is called.
• Have MemoWise listen for changes to the ancestor chain and re-module_eval some/all methods to avoid index conflicts

• Create the README badge+link, and setup automatic generation on RubyDoc.info for both Github project and (later) published gem
• Instrument the project’s development dependencies so that we can generate docs exactly like RubyDoc.info locally
• Backfill docs so that they look good on RubyDoc.info
• Backfill docs for meta-programmed methods as well
• Take a pass to ensure the YARD linking (to classes and methods) is working
• Ensure code snippets are rendered with Ruby highlighting
• Try to add test coverage of documentation example code snippets, e.g. with https://github.com/p0deje/yard-doctest

class Problem
  prepend MemoWise
end

> Marshal.dump(Problem.new)
Traceback (most recent call last):
        2: from (irb):4
        1: from (irb):4:in `dump'
TypeError (can't dump hash with default proc)

The issue may be MemoWise's use of a default proc on the memo hash.

#189 removes the use of fetch in the main memo_wise logic. We should replicate this removal in #preset_memo_wise, #reset_memo_wise and references to fetch_key.

See: #189 (comment)

• Explicit block, raise an argument error at setup of memoization
• Implicit block, passed in, ignore for the purposes of memoization?
  • Why: We’d have to check block_given? at every call, which is an unacceptable performance cost to ensure we are rejecting something we documented we don’t support
• Document in API docs and README that we don’t support memoizing methods which take blocks
  • Could suggest as a workaround defining the method to take a proc arg as a parameter which we can memoize (add a spec here to verify this works as expected)
    • is this necessary?

Give the ability to change the state of a method back to no memoization (i.e. before they called memo_wise).

• Remove existing internal and third-party memoization code in the process

Microbenchmarking shows that array accesses are faster than hash accesses (which makes sense). The tricky thing is how to change e.g. the initialize method to set up something like @_memo_wise = [SENTINEL, {}, {}, SENTINEL, ...]

For no-args methods, we'd need to either:

• use a sentinel value because otherwise we can't distinguish between not-memoized and memoized-as-nil, or
• continue using a separate hash (e.g. @_memo_wise_no_args) instead of the array

NOTE: The reason we can't do something like:

def no_args
  return @memoize_no_args if defined?(@memoize_no_args)

  @memoize_no_args = super
end

is it doesn't work with frozen objects.

• update tagline in gemspec and on GitHub: The wise choice for Ruby memoization
• add feature list/unique selling points of our gem:
  • Highest performance + Rigorous benchmarking
  • Preset
  • reset
  • Frozen instances (value objects)
  • Test coverage
  • Well-documented API
• Add other GitHub badges (see examples here: https://github.com/JacobEvelyn/friends)

I just upgraded from v1.1.0 to v1.2.0 and there seems to be a regression with methods returning booleans. I will see tomorrow if I can write a failing test.

Ensure we're at 100% code coverage for our specs!

Also, mandate 100% branch coverage (see #59 for more context)

Dependabot couldn't parse the config file at .dependabot/config.yml. The error raised was:

(<unknown>): did not find expected key while parsing a block mapping at line 2 column 1

Please ensure the config file is a valid YAML file. An online YAML linter is available here.

I shared this gem on Reddit and someone ask for thread safety:
https://old.reddit.com/r/rails/comments/oglovy/introducing_memowise/

I am not very concerned for my own use but would be good to have something for it.

Do this after 1.0.0 is released to RubyGems

Seem that after d768e10 inheritance is not handled properly, because used indices are mixed up between classes.

require 'memo_wise'

class Parent
  prepend MemoWise

  def bar
    'bar'
  end
  memo_wise :bar
end

class Child < Parent
  def foo
    'foo'
  end
  memo_wise :foo
end

child = Child.new
pp child.foo # initialize Child sentinels
pp child.bar # expected 'bar', got 'foo' because Child sentinels used instead of Parent

Via @JacobEvelyn on Slack:

My branch to port Rainbow uncovered another MemoWise bug 🙂 This line should use respond_to?(method_name, true) instead of respond_to?(method_name) because by default it does not include private methods.

I don't have a ton of time this week—would either of you have time to make a fix?

Squiggly heredocs seem like the "more correct" way to use multiline strings for our use case, and they're supported in all Ruby versions we support, so we should switch to using them everywhere.

• Include a badge in the README
• set a minimum amount below which will trigger test failure
  • should this be an absolute amount or a relative change or both?

We encountered a case in a complex app where we saw something like the following:

class ExampleClass
  include ExampleModule

  def example_method
    true
  end
  memo_wise :example_method
end

module ExampleModule
  prepend MemoWise
  extend ActiveSupport::Concern

  included do
    def example_method
      false
    end
  end
end

ExampleClass.new.example_method
# => true

And we noticed that if instead we have:

class ExampleClass
  prepend MemoWise # This line added
  include ExampleModule

  ...
end

# Now:
ExampleClass.new.example_method
# => false

but if we move the prepend MemoWise after the include ExampleModule we get the original behavior again. Something about calling prepend MemoWise first modifies the ancestor chain in a way I do not understand.

We should investigate and see if this is expected Ruby behavior, a bug in MemoWise, or a bug in the Ruby interpreter.

• make sure this is in the gemspec
• make sure all supported versions are tested

This was a suggestion made by @sampersand in the RubyConf Discord channel for our recent talk. The idea is that instead of using @_memo_wise for all method types, we'd use a different instance variable for each method, like @_memo_wise_method_#{method_name}. This should save us an array lookup for a slight performance boost.

There are a few challenges:

• we'd need to avoid name collisions, for example when memoizing both a data? and a data! and a data method
  • probably a simple .sub("?", "__qmark__").sub("!", "__bang__") would be sufficient
  • @jemmaissroff may have insight into whether there's a max instance variable length to be worried about (or a length at which performance decreases)
    • if so, we could instead use a scheme like @_memo_wise_method_#{counter} or use UUIDs, etc.
• to support frozen objects we need to ensure that these instance variables are initialized to empty hashes before freezing
  • this could probably be done either in an overridden initialize or freeze method
• this approach will not support resetting and presetting zero-arity methods on frozen objects
  • a simple path forward would be to continue using our array for zero-arity methods
  • are there good alternatives?

Would love discussion on this idea and its tradeoffs!

require 'memo_wise'

class C1
  prepend MemoWise
  def method_one
    1
  end
  memo_wise :method_one
end

module M1
  prepend MemoWise
  def method_two
    2
  end
  memo_wise :method_two
end

class C2 < C1
  include M1
  def method_three
    3
  end
  memo_wise :method_three
end

Results in:

/Users/randy.stoller/.rvm/gems/ruby-3.0.2/gems/memo_wise-1.3.0/lib/memo_wise/internal_api.rb:219:in `class_variable_get': class variable @@_memo_wise_index_counter of M1 is overtaken by C1 (RuntimeError)
	from /Users/randy.stoller/.rvm/gems/ruby-3.0.2/gems/memo_wise-1.3.0/lib/memo_wise/internal_api.rb:219:in `next_index!'
	from /Users/randy.stoller/.rvm/gems/ruby-3.0.2/gems/memo_wise-1.3.0/lib/memo_wise.rb:181:in `memo_wise'
	from test.rb:24:in `<class:C2>'
	from test.rb:19:in `<main>'

This worked in 1.1.0 and only happens if the superclass, subclass and module all memo_wise something.

Your dependency file specified a branch or reference for panolint, but Dependabot couldn't find it at the project's source. Has it been removed?

For Ruby dependencies, this can be caused by a branch specified in your Gemfile being deleted at the source, or having been rebased, so the commit reference in your Gemfile.lock is no longer included in the branch. In that case, it can be fixed by running bundler update panolint locally.

View the update logs.

We are using Github Actions rather than Travis CI, this config is stale and unused, isn't it?

Dependabot couldn't find a gems.rb for this project.

Dependabot requires a gems.rb to evaluate your project's current Ruby dependencies. It had expected to find one at the path: /.overcommit/gems.rb.

If this isn't a Ruby project, or if it is a library, you may wish to disable updates for it in the .dependabot/config.yml file in this repo.

View the update logs.

• test against the last published version of the gem (1.8.0)

Including memo_wise, reset_memo_wise, preset_memo_wise

Our CHANGELOG has some inconsistent formatting that could be improved. For example:

• Tenses: do we use fix or fixed or fixes?
• Subheadings: should all versions use the same subheadings?
• PR links: can we link to associated PRs as mentioned in #241 (review) ?
• Diff links: is it clearer to put diff links inline instead of at the bottom of the doc? (I recently released a new version and didn't realize I needed to add that so the link was broken.)

Before reaching v1.0, we'd like to move from the original code layout of "one huge memo_wise.rb for the code, and another huge memo_wise_spec.rb for the tests", and towards a model of extracted code and specs that are individually more approachable and readable.

Consider:

module Speak
  prepend MemoWise

  def speak
    "speaking!"
  end
  memo_wise :speak
end

class Person
   include Speak
  
   def initialize(word = "hey")
     @word = word
   end

   def talk
     @word
   end
end

p = Person.new("hello")

p.talk
#=> "hello"

p.speak
#=> "speaking!"

What happens when we include modules which are using MemoWise into other classes?

Similar to how we only define inherited when we need it, we should only define initialize and allocate where needed.

It would also be nice to do this for reset_memo_wise and preset_memo_wise as well—we don't always need them on both the class and instance level.