Define [nil] as a constant under JekyllFeed

[ashmaroli]

To avoid unnecessary array allocations due to iterating through collections

[mattr-]

Is there any way that you can provide numbers for this change? I'm all for avoiding array allocations but concrete numbers for the improvement this provides would be ✨

[ashmaroli]

Is there any way that you can provide numbers for this change?

There might be a way using ObjectSpace.. not sure though..

[mattr-]

I'd definitely like to see some numbers before merging this, just so that we have concrete proof that this is an improvement.

[pathawks]

This makes the code less readable/maintainable.

If the performance improvement is significant, that seems like a problem with the Ruby interpreter.

[ashmaroli]

If the performance improvement is significant

The argument was not about improving performance, but about optimizing memory usage due to avoidable array allocations.
The "theory" behind my stance can be drawn parallel to how non-frozen string literals with identical characters are a new object with every invocation:

5.times do
  puts "foo".object_id
  puts [nil].object_id
  puts
end

# 22747908
# 22747836
#
# 22747800
# 22747764
#
# 22747728
# 22747692
#
# 22747656
# 22747620
#
# 22747584
# 22747548

[ashmaroli]

@pathawks Would opting for a local variable be okay to you?

    def generate(site)
      @site = site
      nil_array = [nil]

      collections.each do |name, meta|
        Jekyll.logger.info "Jekyll Feed:", "Generating feed for #{name}"
        (meta["categories"] + nil_array).each do |category|
          path = feed_path(:collection => name, :category => category)
          next if file_exists?(path)

          @site.pages << make_page(path, :collection => name, :category => category)
        end
      end
    end

[pathawks]

Do we need to optimize for memory usage? I haven’t seen any complaints about Jekyll using too much memory. 🤷🏼‍♂️

[mattr-]

optimizing memory allocations also optimizes performance, since you spend less time doing memory allocations in general. it's impossible to determine if this PR is worth the tradeoff without benchmarks and metrics though.

[ashmaroli]

@mattr- Benchmarking the change doesn't show any gain from this change.
If using a constant reduces maintainability, are you fine with using a local variable instead:
#262 (comment)

[mattr-]

Benchmarking the change doesn't show any gain from this change.

That's surprising. If there's no gain, I think it's best to leave the code as is. Another approach would be to revert the code change and add a comment explaining that we've tried optimizing this by eliminating the extra memory allocations and it had no effect.

[ashmaroli]

Benchmark script:

require 'benchmark/ips'

COLLECTIONS = ('a'..'z').to_a.each_with_object({}) do |memo, hash|
  hash[memo] = Hash['categories', %w(lorem ipsum dolor sit amet)]
end

NIL_ARRAY = [nil].freeze

def constant
  COLLECTIONS.each do |name, meta|
    (meta['categories'] + NIL_ARRAY).each do |category|
      File.join(category.to_s, 'foo', 'bar')
    end
  end
end

def literal
  COLLECTIONS.each do |name, meta|
    (meta['categories'] + [nil]).each do |category|
      File.join(category.to_s, 'foo', 'bar')
    end
  end
end

Benchmark.ips do |x|
  x.report('constant array') { constant }
  x.report('literal array')  { literal }
  x.compare!
end

[ashmaroli]

I wrote a script to profile object allocations as well.

Script

# frozen_string_literal: true

require 'memory_profiler'
require 'fileutils'

COLLECTIONS = ('a'..'z').to_a.each_with_object({}) do |memo, hash|
  hash[memo] = Hash['categories', %w(lorem ipsum dolor sit amet)]
end

NIL_ARRAY = [nil].freeze

#

def constant
  COLLECTIONS.each do |name, meta|
    (meta['categories'] + NIL_ARRAY).each do |category|
      File.join(category.to_s, 'foo', 'bar')
    end
  end
end

def literal
  COLLECTIONS.each do |name, meta|
    (meta['categories'] + [nil]).each do |category|
      File.join(category.to_s, 'foo', 'bar')
    end
  end
end

#

module Profiler
  TEMP_DIR = File.expand_path('.profile', __dir__).freeze
  private_constant :TEMP_DIR

  def self.boot(count = 1)
    FileUtils.mkdir_p(TEMP_DIR)
    @count = count.to_i
    @count = 1 if @count.zero?
    times = @count == 1 ? 'just once' : "#{@count} times"
    puts "\nAllocated objects by class:\n(Looping #{times})"
    yield self
  ensure
    FileUtils.rm_r(TEMP_DIR)
  end

  def self.profile(filename, &block)
    report_file = File.join(TEMP_DIR, filename)
    label = "#{filename}:"
    puts "\n#{label}\n#{'-' * label.length}"

    report = MemoryProfiler.report { @count.times(&block) }
    report.pretty_print(to_file: report_file)
    File.read(report_file) =~ %r!allocated objects by class\s*-+\s(.*?)\sretained memory by gem!m
    puts $1
  end
end

Profiler.boot(ARGV[0] || 1000) do |x|
  x.profile("literal")  { literal }
  x.profile("constant") { constant }
end

Results

Allocated objects by class:
(Looping 1000 times)

literal:
--------
    208000  Array
    182000  String

constant:
---------
    182000  Array
    182000  String