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

Merged
merged 1 commit into from Jan 10, 2016

Conversation

Projects
None yet
3 participants
@envygeeks
Contributor

envygeeks commented Jan 10, 2016

No description provided.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 10, 2016

Contributor

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

Contributor

envygeeks commented Jan 10, 2016

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

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 10, 2016

Contributor

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.

Contributor

envygeeks commented Jan 10, 2016

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}")

This comment has been minimized.

@parkr

parkr Jan 10, 2016

Member

We should raise some Jekyll-specific error here.

@parkr

parkr Jan 10, 2016

Member

We should raise some Jekyll-specific error here.

lib/jekyll/generator.rb
@@ -1,4 +1,5 @@
module Jekyll
class Generator < Plugin
+ #

This comment has been minimized.

@parkr

parkr Jan 10, 2016

Member

Please remove

@parkr

parkr Jan 10, 2016

Member

Please remove

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2016

Member

Way faster! Thanks! 🎉

Member

parkr commented Jan 10, 2016

Way faster! Thanks! 🎉

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2016

Member

@jekyllbot: merge +dev

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot deleted the pr/remove-objectspace-and-use-inherited branch Jan 10, 2016

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.