Use method_defined? instead of instance_methods lookup

[marshall-lee]

This one is related to #336 and #307 and solves problem more efficient and simple (no memoization).

There is no need to call instance_methods at all because Ruby already has method_defined? that internally uses hash lookup (st_lookup from st.c). After all, it doesn't create new array/set objects that is good too.

[marshall-lee]

It's also worth to not convert every string to symbol — there's no need to do it now.

[hubert]

the behavior of method_defined? vs instance_methods.include? aren't equivalent. method_defined? would also return true if the method were defined by one of it's superclasses, while instance_methods.include? would not.

from my reading of the code, the latter behavior is what we want. i took a quick look at Module and don't see an equivalent. do you know of something that i missed?

[marshall-lee]

the behavior of method_defined? vs instance_methods.include? aren't equivalent. method_defined? would also return true if the method were defined by one of it's superclasses, while instance_methods.include? would not.

No, instance_methods traverses all superclasses too:

class X
  def m
  end
end

class Y < X; end

puts Y.instance_methods.include?(:m) # outputs 'true'

# However:

puts Y.instance_methods(false).include?(:m) # outputs 'false'

But instance_methods without false is being calling in our case.

from my reading of the code, the latter behavior is what we want

I don't think so. What if I want to inherit my model class from another?

[marshall-lee]

I created a benchmark branch in my fork — it just contains a benchmark rake task that runs User.all.to_a with mocked faraday response so it measures only CPU work without any real networking. After this i created a separate branches for all relevant PRs and merged them with benchmark branch.

So we have 4 variants:

• benchmark — current master with benchmarks.
• benchmark-instance_methods_by_generalassembly — the Memoize user-defined methods during Her attribute definition #307 with benchmarks
• benchmark-instance_methods_by_faisalmansoor — the use a set rather than array to test existance of instance methods #336 with benchmarks
• benchmark-instance_methods_by_marshall_lee — this one

Here the benchmarks from my laptop:

master            0.350000   0.000000   0.350000 (  0.352151)
generalassembly   0.160000   0.000000   0.160000 (  0.169817)
faisalmansoor     0.230000   0.000000   0.230000 (  0.234990)
this one          0.110000   0.000000   0.110000 (  0.103908)

This one performs better!

[marshall-lee]

Also we can see that @faisalmansoor's solution (#336 — that constructs a Set for memoization) is slower than @generalassembly's (#307 — that memoizing with Array). I think this is because of Set constructing overhead but maybe i'm wrong.

[hubert]

should have tried it out rather than just reading the doc, which lead to me overlooking the diff between modules and classes.

re: what the behavior should be

class A
  attr_accessor :foo
end

class B < A
  include Her::Model

  attributes :foo
end

it's a corner case, but here, i'd expect the attribute foo to overwrite the foo= method. that said, the behavior is not changing so no need to try and deal with now.

the PR code gets exercised indirectly in specs, but could you add specs to verify the attributes method is doing what we expect in the case where there isn't a user defined version of the setter and predicate method?

GTG once that's in place.

[hubert]

💘 for the benchmark

[marshall-lee]

ok I will add it soon.

[faisalmansoor]

@marshall-lee can you explain your benchmark a little bit more, how many objects did you test with and how many attributes each object had? array include is implemented using a linear search, I don't think just memorizing the array will solve the problem, even with memorization we will end up with a quadratic runtime for this function. I think we should use a set or method_defined function, they both will result in at max O(n lg n) runtime for this function.

VALUE
rb_ary_includes(VALUE ary, VALUE item)
{
    long i;

    for (i=0; i<RARRAY_LEN(ary); i++) {
        if (rb_equal(RARRAY_PTR(ary)[i], item)) {
            return Qtrue;
        }
    }
    return Qfalse;
}

[marshall-lee]

@faisalmansoor

how many objects did you test with and how many attributes each object had

benchmark code is open, i provided the link above (benchmark and all benchmark-* branches). The benchmark rake task itself is here: https://github.com/marshall-lee/her/blob/benchmark/Rakefile#L13

In this benchmark I emulate a case when fetching from server an array of 1000 objects. Each object has three properties: id, email, name.

[marshall-lee]

I don't think just memorizing the array will solve the problem

O(lgN * N) vs O(N * N) just tells us about a rate of growth. Sure, solution with Set will be better than linear search but only when N is sufficiently large. In our concrete case this N is not so big and probably will never be — it's just a number of methods of a class. So our problem is not a large value of N.

[marshall-lee]

By the way, Set lookup is not a O(lgN) — it's simply O(1).

[marshall-lee]

@hubert

the PR code gets exercised indirectly in specs

Sometimes directly and sometimes indirectly. Specs for setters/getters are already in spec/model/dirty_spec.rb. Are they sufficient?

I just wrote my spec for attributes class method in spec/model/attributes_spec.rb but not yet commited it because I realized it's redundant.

UPD:

  context "attributes class method" do
    before do
      spawn_model 'Foo::User' do
        attributes :fullname, :document
      end
    end

    context "instance" do
      subject { Foo::User.new }

      it { should respond_to(:fullname) }
      it { should respond_to(:fullname=) }
      it { should respond_to(:fullname?) }
    end

    it "defines setter that affects attributes" do
      user = Foo::User.new
      user.fullname = 'Vladimir Kochnev'
      user.attributes[:fullname].should eq('Vladimir Kochnev')
    end
  end

[marshall-lee]

About a corner case discovered by you: yeah it's a big problem. I think in next major version we should split attributes class macro and attributes definition that's being calling indirectly. So attributes macro should always override methods defined before but indirect call should check for existence with method_defined?.

[hubert]

@marshall-lee the reason i asked for specs is the behavior of the attributes method is i think it will be clearer to gem users if the behavior is spec-ed explicitly. the current specs also don't check that the predicate method gets defined.

the attribute.to_sym call is also unnecessary since it gets converted back to a string when it gets interpolated. we can deal with that separately if you prefer.

[marshall-lee]

the attribute.to_sym call is also unnecessary since it gets converted back to a string when it gets interpolated. we can deal with that separately if you prefer.

I wanted to do it separately because it's not the only place to fix string <=> symbol transformation. It's also not necessary to do interpolation like :"#{attribute}" because @attributes is already a HashWithIndifferentAccess. This fix deserves separate PR.

[marshall-lee]

I added specs. Are they ok?

[hubert]

yep. looks good.

thanks for your contribution 👍