WIP: Refactor metadata

[myronmarston]

Convert Metadata to use composition rather than inheritance.

• It's best to avoid subclassing core types like Hash.
• Remove runtime module extensions, since that blows MRI's method
  caches and has an affect on perf.
• Instantiation was a two part process: .new followed by process.
  Conceptually, these belong together, so switch to using a single
  method for instantiation.

Two gotchas I discovered:

• The include matcher didn't initially work properly with Metadata.
  It uses an is_a?(Hash) check to know how to treat the object.
  To solve this, we override is_a? to return true for Hash.
• dup didn't work as expected. dup is used to get a copy that can be
  modified without affecting the original; however, dup was simply
  duping the wrapping object, and each retained a reference to the same
  wrapped hash, causing changes on one to show up on the other. I override
  dup and clone to address this.

However...this seems to have a noticeable negative perf impact, in spite of the fact I expected it to have a positive perf impact (due to no longer using module extensions at run time). Here are 3 runs of the test suite against this branch:

➜  rspec-core git:(refactor_metadata) ✗ bin/rspec | grep Finished
Finished in 3.27 seconds
➜  rspec-core git:(refactor_metadata) ✗ bin/rspec | grep Finished
Finished in 3.02 seconds
➜  rspec-core git:(refactor_metadata) ✗ bin/rspec | grep Finished
Finished in 3.12 seconds 

Compare that to master:

➜  rspec-core git:(master) ✗ bin/rspec | grep Finished
Finished in 2.94 seconds
➜  rspec-core git:(master) ✗ bin/rspec | grep Finished
Finished in 2.98 seconds
➜  rspec-core git:(master) ✗ bin/rspec | grep Finished
Finished in 3.05 seconds

I'm not sure why. This seems like a better design to me, but I don't want to merge it if it's going to have that much of an affect on perf. There may be a way to salvage it though.

Thoughts?

/cc @alindeman @JonRowe @soulcutter @samphippen

[coveralls]

Coverage remained the same when pulling c776b88 on refactor_metadata into afee9c1 on master.

[JonRowe]

My own benchmarks...

 |       | master | branch |
 | 2.0.0 |  ~2.5  |  ~2.6  |  
 | 1.9.3 |  ~2.2  |  ~2.4  |
 | 1.8.7 |  ~7.1  |  ~8.8  |

[JonRowe]

I've run into performance issues (normally negligible in total usage) with ruby delegates before, a quick bench mark show that this is still an issue and is probably the cause of any slow down you may be seeing, I seem to remember it was more pronounced on 1.9.2 than 1.9.3 and 2.0.0 etc...

[coveralls]

Coverage remained the same when pulling 929dda2 on refactor_metadata into afee9c1 on master.

[myronmarston]

a quick bench mark show that this is still an issue

Can you post said benchmark? I'd like to see for myself.

[JonRowe]

I pushed a commit with the script I used and the outcomes for me, but I'll repost the outcomes below

# 100000 times - ruby 2.0.0
# [control] straight to obj
# 0.040000   0.000000   0.040000 (  0.038468)
# [delegate] DelegateClass to obj
# 0.230000   0.000000   0.230000 (  0.234356)
# [composed] passed to obj
# 0.070000   0.000000   0.070000 (  0.071021)

[JonRowe]

Did you get a chance to look at the benchmark? WDYT about #1008 ?

[myronmarston]

I haven't had a chance yet. I have some implementation concerns about #1008, and I want to dig into this more before deciding what to do.

[JonRowe]

Fair. I was just gently nudging you ;)

[soulcutter]

Is "It's best to avoid subclassing core types like Hash." based on Steve Klabnik's recent blog? I feel like it's not clear how much you can realistically expect to run into the weirdness he discovered there (I believe his post even says that it works well 99% of the time).

In any case, I wonder how much faster Forwardable may be - I imagine the explicit mapping is faster than delegating everything, but it still may be slower than subclassing the core type.

TLDR; Subclassing Hash may be ok here.

[myronmarston]

@soulcutter -- the stuff Steve brings up is part of it, but I had been thinking about doing this long before that blog post. In general, I nearly always favor composition over inheritance, especially when dealing with core types. More than any specific problems we've had (or not had), I think "don't subclass core types" is sound advice and I'd like rspec's code to be a good "role model" in this regard, so to speak.

That said, I took a look at the delegate logic and I can see why it's so much slower:

https://github.com/ruby/ruby/blob/835c555f7f137746eb6f7cd32bd05a386852fdbe/lib/delegate.rb#L319-L328

One delegation results in a couple extra method sends (e.g. one to __getobj__, one to __send__, one to delete_if) and the delete_if loop in particular will slow stuff down: that's O(N) over the size of the backtrace.

So this approach isn't going to work.

I'm going to close this to continue the discussion on #1008.

[soulcutter]

I generally agree about composition vs inheritance, but I was giving a pragmatic perspective there :) Any reason to leave this code branch around?

[JonRowe]

#1008 pulls into refactor_metadata