tycooon / memery

A gem for memoization in Ruby
MIT License
177 stars 13 forks source link

Repair code to avoid "instance variable not initialized" warning #12

Closed martinstreicher closed 5 years ago

martinstreicher commented 5 years ago

Ruby 2.6.2 generates the instance variable not initialized warning for a variable in this gem. I moved some code around to avoid the error message.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 51


Totals Coverage Status
Change from base Build 50: 0.0%
Covered Lines: 189
Relevant Lines: 189

💛 - Coveralls
martinstreicher commented 5 years ago

No issues found by Rubocop. Specs are green.

tycooon commented 5 years ago

I guess you just partly reverted #10, maybe use defined? operator instead?

martinstreicher commented 5 years ago

I will run a benchmark. I cannot imagine these more time, but let's find out.

AlexWayfer commented 5 years ago

Ruby 2.6.2 generates the instance variable not initialized warning for a variable in this gem.

How to get this?

Oh, I found: --warnings should be added into .rspec.

AlexWayfer commented 5 years ago

Benchmark:

# frozen_string_literal: true

require 'bundler/setup'
Bundler.setup

require 'benchmark'
require 'benchmark/ips'
require 'benchmark/memory'

puts '```ruby'
puts File.read(__FILE__).gsub("\t", '  ')
puts '```'
puts
puts '### Output'
puts
puts '```'

require_relative 'lib/memery'
require_relative 'lib/memery_old'

class Foo
  class << self
    include Memery
    include MemeryOld

    def base_find(char)
      ('a'..'k').find { |letter| letter == char }
    end

    memoize_old def find_old(char)
      base_find(char)
    end

    memoize def find_new(char)
      base_find(char)
    end
  end
end

def memery_old
  Foo.find_old('d')
end

def memery_new
  Foo.find_new('d')
end

def test
  exit unless p (((p memery_old) == (p memery_new)))
end

test

Benchmark.ips do |x|
  x.report('memery_old') { memery_old }
  x.report('memery_new') { memery_new }

  x.compare!
end

Benchmark.memory do |x|
  x.report('memery_old') { 100.times { memery_old } }
  x.report('memery_new') { 100.times { memery_new } }

  x.compare!
end

puts '```'

Output

"d"
"d"
true
Warming up --------------------------------------
          memery_old    28.864k i/100ms
          memery_new    29.188k i/100ms
Calculating -------------------------------------
          memery_old    332.771k (± 4.5%) i/s -      1.674M in   5.041161s
          memery_new    341.168k (± 4.6%) i/s -      1.722M in   5.058746s

Comparison:
          memery_new:   341168.0 i/s
          memery_old:   332771.1 i/s - same-ish: difference falls within error

Calculating -------------------------------------
          memery_old    16.000k memsize (     0.000  retained)
                       400.000  objects (     0.000  retained)
                         3.000  strings (     0.000  retained)
          memery_new    16.000k memsize (     0.000  retained)
                       400.000  objects (     0.000  retained)
                         3.000  strings (     0.000  retained)

Comparison:
          memery_old:      16000 allocated
          memery_new:      16000 allocated - same
tycooon commented 5 years ago

I think we should add the rake benchmark task to this repo 🤔

AlexWayfer commented 5 years ago

By the way, actual master version (with :ttl option) doesn't have such warnings.

AlexWayfer commented 5 years ago

I think we should add the rake benchmark task to this repo thinking

I can try, but it will be without comparison, I guess. Or we need to get the library from master in some way, rename it, etc. …

Or we can just have an example of bench script (it's ignored for git at my comp now).

tycooon commented 5 years ago

I think that without a comparison is OK – you can always run it on master and your branch and compare manually.

AlexWayfer commented 5 years ago

I think that without a comparison is OK – you can always run it on master and your branch and compare manually.

Done in #14.

martinstreicher commented 5 years ago
Warming up --------------------------------------
         test_memery    28.370k i/100ms
Calculating -------------------------------------
         test_memery    302.473k (± 4.4%) i/s -      1.532M in   5.074632s
Calculating -------------------------------------
         test_memery    20.000k memsize (     0.000  retained)
                       500.000  objects (     0.000  retained)
                         3.000  strings (     0.000  retained)
AlexWayfer commented 5 years ago

@martinstreicher, warnings are gone in the current master version. You can check it and close this PR.

martinstreicher commented 5 years ago

I did check...

/Users/mss67/.rvm/gems/ruby-2.6.2@states-machines/bundler/gems/memery-7cb68b9295ed/lib/memery.rb:55: warning: instance variable @_memery_memoized_values not initialized
/Users/mss67/.rvm/gems/ruby-2.6.2@states-machines/bundler/gems/memery-7cb68b9295ed/lib/memery.rb:55: warning: instance variable @_memery_memoized_values not initialized
      finds models that must be advanced

The warning persists because line 55 is the same issue I identified previously.

martinstreicher commented 5 years ago

My Gemfile...

gem 'memery', github: 'tycooon/memery', branch: 'master'

and Gemfile.lock

GIT
  remote: https://github.com/tycooon/memery.git
  revision: 7cb68b9295ed6eee1e02edc4640c61b775115593
  branch: master
  specs:
    memery (1.1.0)
tycooon commented 5 years ago

Thanks for the patch! I fixed a rubocop issue and merged.