Skip to content

WIP: Refactor metadata

gitlab-qa-bot requested to merge refactor_metadata into master

Created by: 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

Merge request reports