Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Conversation

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jul 21, 2013

I think DelegateClass is the cause of the performance degradation you're seeing @myronmarston, give this a try. I tried doing this without directly specifying the methods but that had a similar performance failing.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 3b679e7 on refactor_metadata_no_delegate into 929dda2 on refactor_metadata.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 6d5d7f3 on refactor_metadata_no_delegate into 929dda2 on refactor_metadata.

@JonRowe JonRowe mentioned this pull request Jul 23, 2013
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few concerns here:

  • Several of these methods take blocks and you aren't properly forwarding them. (e.g. fetch and delete).
  • Why only this set of methods? Hash has many other methods, I believe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was what was required to implement the MetaData functionality. Do we want it to behave entirely like a hash, or is it acceptable to provide only our own api? I'm open to restoring the block forwarding though.

@myronmarston
Copy link
Member

I think it's worth looking into doing something like:

require 'forwardable'

class Metadata
  extend Forwardable
  def_delegators :@hash, *Hash.instance_methods
end

Maybe see how that benchmarks?

Alternately, something else I've been mulling is the idea of just using a raw hash, w/o wrapping it or subclassing it or anything...and using it's default_proc to rather than overriding store, and have it delegate to an outside MetadataCalculator (or whatever) object.

Not sure it's worth the effort, though :(. Ultimately, what I care about most is getting rid of the runtime extends that were being used as that causes method caches to be blown away and creates a perf hit.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 01b7b5b on refactor_metadata_no_delegate into 5fe8ea9 on refactor_metadata.

@JonRowe
Copy link
Member Author

JonRowe commented Aug 1, 2013

I added Forwardable to my benchmark/delegate_v_defined.rb benchmark, for me it's about half the time of delegated, but still 2x more than composed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling dda6d1e on refactor_metadata_no_delegate into 5b25672 on refactor_metadata.

@myronmarston
Copy link
Member

@JonRowe -- I think I've decided against this (even though I initially got this ball rolling). Thanks for giving it a shot, though.

@JonRowe JonRowe deleted the refactor_metadata_no_delegate branch December 3, 2013 00:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants