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

Reduce string allocations with better alternatives #7643

Merged
merged 6 commits into from
May 16, 2019

Conversation

ashmaroli
Copy link
Member

Rationales

  • interpolation results in a new object allocation for each call. So, interpolation within an iteration block is wasteful unless the interpolated variable is the block parameter.
  • Regarding the changes to frontmatter_defaults.rb:
    • Private method #path_is_subpath? checks a derived string against a string value derived from the second argument to the method. Instead of generating a new string from a Pathname object, its better to just pass String objects directly.
      def path_is_subpath?(path, parent_path)
      path.ascend do |ascended_path|
      return true if ascended_path.to_s == parent_path.to_s
      end
    • slashed_coll_dir = collections_dir.empty? ? "/" : "#{collections_dir}/" is better since it avoids allocating interpolated strings for default configuration. Surprisingly, its a lot faster too..!
      # frozen_string_literal: true
      
      require 'benchmark/ips'
      
      def interpolate(collections_dir)
        "#{collections_dir}/"
      end
      
      def conditional(collections_dir)
        collections_dir.empty? ? "/" : "#{collections_dir}/"
      end
      
      ["", "foo"].each do |dir|
        return unless interpolate(dir) == conditional(dir)
      
        Benchmark.ips do |x|
          x.report("interpolate '#{dir}'") { interpolate(dir) }
          x.report("conditional '#{dir}'") { conditional(dir) }
          x.compare!
        end
      end
      Warming up --------------------------------------
            interpolate ''    73.729k i/100ms
            conditional ''    88.071k i/100ms
      Calculating -------------------------------------
            interpolate ''      1.991M (A± 0.7%) i/s -      9.953M in   5.000580s
            conditional ''      3.412M (A± 2.9%) i/s -     17.086M in   5.013017s
      
      Comparison:
            conditional '':  3411816.5 i/s
            interpolate '':  1990549.5 i/s - 1.71x  slower
      
      Warming up --------------------------------------
         interpolate 'foo'    73.693k i/100ms
         conditional 'foo'    72.454k i/100ms
      Calculating -------------------------------------
         interpolate 'foo'      1.992M (A± 0.6%) i/s -     10.022M in   5.031876s
         conditional 'foo'      1.911M (A± 0.8%) i/s -      9.564M in   5.006282s
      
      Comparison:
         interpolate 'foo':  1991828.9 i/s
         conditional 'foo':  1910513.6 i/s - 1.04x  slower
      

Context

String allocations will be further reduced by shipping #7406

@ashmaroli ashmaroli requested a review from a team May 3, 2019 12:42
@ashmaroli
Copy link
Member Author

Profiler summary

--- master branch https://travis-ci.org/jekyll/jekyll/jobs/532959217
+++ PR branch     https://travis-ci.org/jekyll/jekyll/jobs/532974783

- Total allocated: 532.65 MB (5301707 objects)
- Total retained:  18.34 MB (96237 objects)
+ Total allocated: 532.07 MB (5287525 objects)
+ Total retained:  18.34 MB (96241 objects)

lib/jekyll/entry_filter.rb Show resolved Hide resolved
@mattr-
Copy link
Member

mattr- commented May 16, 2019

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit cbfdeae into jekyll:master May 16, 2019
jekyllbot added a commit that referenced this pull request May 16, 2019
@ashmaroli ashmaroli deleted the reduce-string-allocations branch May 16, 2019 14:53
@jekyll jekyll locked and limited conversation to collaborators May 15, 2020
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.

3 participants