Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ObectSpace dumping and start using inherited, it's faster. #4342

Merged
merged 1 commit into from Jan 10, 2016

Conversation

envygeeks
Copy link
Contributor

No description provided.

@envygeeks envygeeks force-pushed the pr/remove-objectspace-and-use-inherited branch from 561414c to 4f20e3e Compare January 10, 2016 19:41
@envygeeks
Copy link
Contributor Author

Hulk: benchmark +old-v-new : Prove to me my theory is right.

@envygeeks
Copy link
Contributor Author

I have done as you requested:

Calculating -------------------------------------
                   A   204.000  i/100ms
                   B     6.063k i/100ms
-------------------------------------------------
                   A      2.019k (±10.6%) i/s -      9.996k
                   B     70.913k (± 1.0%) i/s -    357.717k

Comparison:
                   B:    70913.0 i/s
                   A:     2018.6 i/s - 35.13x slower

Here is my source because I know you bang on about it constantly being important:

require "benchmark/ips"
require "set"

class A
  def self.descendants
    descendants = []
    ObjectSpace.each_object(singleton_class) do |k|
      descendants.unshift k unless k == self
    end
    descendants
  end
end

class B
  def self.inherited(const)
    return catch_inheritance(const) do |const_|
      catch_inheritance(const_)
    end
  end

  #

  def self.catch_inheritance(const)
    const.define_singleton_method :inherited do |const_|
      (@children ||= Set.new).add const_
      if block_given?
        yield const_
      end
    end
  end

  #

  def self.descendants
    @children ||= Set.new
    out = @children.map(&:descendants)
    out << self unless superclass == B
    Set.new(out).flatten
  end
end

Object.const_set(:A1, Class.new(A))
Object.const_set(:A2, Class.new(A))
Object.const_set(:A3, Class.new(A))
Object.const_set(:A4, Class.new(A))
Object.const_set(:A5, Class.new(A))
Object.const_set(:A6, Class.new(A))
Object.const_set(:A7, Class.new(A))
Object.const_set(:B1, Class.new(B))
Object.const_set(:B2, Class.new(B))
Object.const_set(:B3, Class.new(B))
Object.const_set(:B4, Class.new(B))
Object.const_set(:B5, Class.new(B))
Object.const_set(:B6, Class.new(B))
Object.const_set(:B7, Class.new(B))

Benchmark.ips do |x|
  x.report("A") { A.descendants }
  x.report("B") { B.descendants }
  x.compare!
end

You started the race slow but came out ahead so far I'm pretty sure you rigged the race car some how. You won though until I can prove otherwise.

def find_converter_instance(klass)
converters.find { |c| c.class == klass } || proc { raise "No converter for #{klass}" }.call
converters.find { |klass_| klass_.instance_of?(klass) } || \
raise("No Converters found for #{klass}")
Copy link
Member

Choose a reason for hiding this comment

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

We should raise some Jekyll-specific error here.

@parkr
Copy link
Member

parkr commented Jan 10, 2016

Way faster! Thanks! 🎉

@envygeeks envygeeks force-pushed the pr/remove-objectspace-and-use-inherited branch from 4f20e3e to 70f741b Compare January 10, 2016 20:11
@parkr
Copy link
Member

parkr commented Jan 10, 2016

@jekyllbot: merge +dev

jekyllbot added a commit that referenced this pull request Jan 10, 2016
@jekyllbot jekyllbot merged commit 3c0a138 into master Jan 10, 2016
@jekyllbot jekyllbot deleted the pr/remove-objectspace-and-use-inherited branch January 10, 2016 20:42
jekyllbot added a commit that referenced this pull request Jan 10, 2016
@envygeeks envygeeks added this to the 3.1 milestone Jan 10, 2016
@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants