Cache Jekyll.sanitized_path

[fauno]

This is a 🔨 code refactoring.

Summary

I was running stackprof over a site with ~600 posts and noticed Jekyll.sanitize_path was being called many times over so I tried caching responses. Benchmarks logs for jekyll-sanitize-path before and after:

Before:

Warming up --------------------------------------
with no questionable path
                         6.977k i/100ms
with a single-part questionable path
                         6.620k i/100ms
with a multi-part questionable path
                         6.350k i/100ms
with a single-part traversal path
                         6.544k i/100ms
with a multi-part traversal path
                         6.314k i/100ms
with the exact same paths
                       402.619k i/100ms
with a single-part absolute path including the base_directory
                         6.656k i/100ms
with a multi-part absolute path including the base_directory
                         6.443k i/100ms
with a single-part traversal path including the base_directory
                        12.621k i/100ms
with a multi-part traversal path including the base_directory
                         5.848k i/100ms
Calculating -------------------------------------
with no questionable path
                         70.152k (± 1.9%) i/s -    355.827k in   5.074255s
with a single-part questionable path
                         66.758k (± 2.7%) i/s -    337.620k in   5.061671s
with a multi-part questionable path
                         63.044k (± 4.2%) i/s -    317.500k in   5.046474s
with a single-part traversal path
                         66.523k (± 2.6%) i/s -    333.744k in   5.020730s
with a multi-part traversal path
                         59.564k (±17.5%) i/s -    290.444k in   5.133958s
with the exact same paths
                          3.384M (±19.7%) i/s -     16.507M in   5.128966s
with a single-part absolute path including the base_directory
                         52.928k (±20.3%) i/s -    252.928k in   5.009928s
with a multi-part absolute path including the base_directory
                         57.189k (±21.4%) i/s -    277.049k in   5.216751s
with a single-part traversal path including the base_directory
                         82.299k (±22.6%) i/s -    391.251k in   5.068463s
with a multi-part traversal path including the base_directory
                         37.547k (±12.6%) i/s -    187.136k in   5.087265s

After:

Warming up --------------------------------------
with no questionable path
                        69.426k i/100ms
with a single-part questionable path
                        63.989k i/100ms
with a multi-part questionable path
                        67.406k i/100ms
with a single-part traversal path
                        68.679k i/100ms
with a multi-part traversal path
                        63.054k i/100ms
with the exact same paths
                       411.366k i/100ms
with a single-part absolute path including the base_directory
                        28.353k i/100ms
with a multi-part absolute path including the base_directory
                        29.346k i/100ms
with a single-part traversal path including the base_directory
                         9.966k i/100ms
with a multi-part traversal path including the base_directory
                        40.974k i/100ms
Calculating -------------------------------------
with no questionable path
                        674.568k (± 2.8%) i/s -      3.402M in   5.047108s
with a single-part questionable path
                        656.353k (± 4.1%) i/s -      3.327M in   5.079440s
with a multi-part questionable path
                        642.866k (± 3.0%) i/s -      3.235M in   5.037611s
with a single-part traversal path
                        655.010k (± 2.8%) i/s -      3.297M in   5.037056s
with a multi-part traversal path
                        618.577k (± 2.9%) i/s -      3.153M in   5.101243s
with the exact same paths
                          3.771M (± 9.5%) i/s -     18.923M in   5.070652s
with a single-part absolute path including the base_directory
                        285.799k (± 3.5%) i/s -      1.446M in   5.066518s
with a multi-part absolute path including the base_directory
                        282.806k (± 5.1%) i/s -      1.438M in   5.101322s
with a single-part traversal path including the base_directory
                         97.885k (± 1.6%) i/s -    498.300k in   5.092095s
with a multi-part traversal path including the base_directory
                        400.997k (± 2.5%) i/s -      2.008M in   5.010275s

[ashmaroli]

Thank you for this submission, @fauno. I love optimization pull requests.
However, I have some reservations here though.

Jekyll is the global module. I'm not very confident about instance variables in a global module.
That aside, earlier the result could be freely mutated. Now since it is cached, mutating the resulting string in one place will affect result elsewhere..

[fauno]

Thanks for the review! Would it be ok to move the cache to a private method in the module? Since it's caching results I don't find it so problematic to share the cache/instance var, since the result is idempotent anyway, no?

As per mutability, I didn't take that into account, it could return a duplicated string, the benchmark looks better:

benchmark	with patch and dup result	with patch	without patch
with no questionable path	72.103k i/100ms	69.426k i/100ms	6.977k i/100ms
with a single-part questionable path	73.170k i/100ms	63.989k i/100ms	6.620k i/100ms
with a multi-part questionable path	72.014k i/100ms	67.406k i/100ms	6.350k i/100ms
with a single-part traversal path	68.148k i/100ms	68.679k i/100ms	6.544k i/100ms
with a multi-part traversal path	67.653k i/100ms	63.054k i/100ms	6.314k i/100ms
with the exact same paths	384.003k i/100ms	411.366k i/100ms	402.619k i/100ms
with a single-part absolute path including the base_directory	30.057k i/100ms	28.353k i/100ms	6.656k i/100ms
with a multi-part absolute path including the base_directory	28.053k i/100ms	29.346k i/100ms	6.443k i/100ms
with a single-part traversal path including the base_directory	9.830k i/100ms	9.966k i/100ms	12.621k i/100ms
with a multi-part traversal path including the base_directory	43.127k i/100ms	40.974k i/100ms	5.848k i/100ms
with no questionable path	740.184k (± 1.4%) i/s - 3.749M in 5.066490s	674.568k (± 2.8%) i/s - 3.402M in 5.047108s	70.152k (± 1.9%) i/s - 355.827k in 5.074255s
with a single-part questionable path	731.445k (± 0.7%) i/s - 3.658M in 5.002016s	656.353k (± 4.1%) i/s - 3.327M in 5.079440s	66.758k (± 2.7%) i/s - 337.620k in 5.061671s
with a multi-part questionable path	686.939k (±12.2%) i/s - 3.385M in 5.055924s	642.866k (± 3.0%) i/s - 3.235M in 5.037611s	63.044k (± 4.2%) i/s - 317.500k in 5.046474s
with a single-part traversal path	722.345k (± 1.4%) i/s - 3.612M in 5.001173s	655.010k (± 2.8%) i/s - 3.297M in 5.037056s	66.523k (± 2.6%) i/s - 333.744k in 5.020730s
with a multi-part traversal path	659.161k (± 5.8%) i/s - 3.315M in 5.049436s	618.577k (± 2.9%) i/s - 3.153M in 5.101243s	59.564k (±17.5%) i/s - 290.444k in 5.133958s
with the exact same paths	3.821M (± 2.8%) i/s - 19.200M in 5.028773s	3.771M (± 9.5%) i/s - 18.923M in 5.070652s	3.384M (±19.7%) i/s - 16.507M in 5.128966s
with a single-part absolute path including the base_directory	300.236k (± 1.4%) i/s - 1.503M in 5.006624s	285.799k (± 3.5%) i/s - 1.446M in 5.066518s	52.928k (±20.3%) i/s - 252.928k in 5.009928s
with a multi-part absolute path including the base_directory	281.253k (± 1.5%) i/s - 1.431M in 5.088109s	282.806k (± 5.1%) i/s - 1.438M in 5.101322s	57.189k (±21.4%) i/s - 277.049k in 5.216751s
with a single-part traversal path including the base_directory	101.875k (± 1.8%) i/s - 511.160k in 5.019353s	97.885k (± 1.6%) i/s - 498.300k in 5.092095s	82.299k (±22.6%) i/s - 391.251k in 5.068463s
with a multi-part traversal path including the base_directory	429.309k (± 1.7%) i/s - 2.156M in 5.024406s	400.997k (± 2.5%) i/s - 2.008M in 5.010275s	37.547k (±12.6%) i/s - 187.136k in 5.087265s

[ashmaroli]

Thanks for the numbers! Very inspiring!

since the result is idempotent anyway, no?

In a way, yes this is true.
But since this is Ruby, things can get complicated.

For example consider the following hypothetical plugin:

class Foo
  extend Jekyll

  @sanitization_path_cache = {}

  def self.clarify(input)
    @sanitization_path_cache[input] ||= begin
      lorem = call_business_logic(input)
      sanitized_path(base, lorem)
    end
  end
end

The code above would work as the author expected until Jekyll-4.1.
With Jekyll 4.2, it may not be so..
Additionally, @sanitization_path_cache in the plugin above need not be just a Hash but a specialized object in itself as well.

The reason I'm saying this is not to discourage you but to err on the safer side. In the past, I myself had used instance variables as cache in a module (see Jekyll::Filters#where). Luckily, only one user had problems because of it (to my knowledge).

I didn't take that into account, it could return a duplicated string

Ah! That's a nice solution.
If you're returning a duplicated string, you could also replace File.join(*) with Jekyll::PathManager.join(*) for additional optimization.

[fauno]

Looking good :)

benchmark	without patch	with patch	with patch and dup result	patch + dup + pathmanager
with no questionable path	6.977k i/100ms	69.426k i/100ms	72.103k i/100ms	75.574k i/100ms
with a single-part questionable path	6.620k i/100ms	63.989k i/100ms	73.170k i/100ms	74.534k i/100ms
with a multi-part questionable path	6.350k i/100ms	67.406k i/100ms	72.014k i/100ms	70.119k i/100ms
with a single-part traversal path	6.544k i/100ms	68.679k i/100ms	68.148k i/100ms	72.508k i/100ms
with a multi-part traversal path	6.314k i/100ms	63.054k i/100ms	67.653k i/100ms	68.073k i/100ms
with the exact same paths	402.619k i/100ms	411.366k i/100ms	384.003k i/100ms	412.564k i/100ms
with a single-part absolute path including the base_directory	6.656k i/100ms	28.353k i/100ms	30.057k i/100ms	30.586k i/100ms
with a multi-part absolute path including the base_directory	6.443k i/100ms	29.346k i/100ms	28.053k i/100ms	30.069k i/100ms
with a single-part traversal path including the base_directory	12.621k i/100ms	9.966k i/100ms	9.830k i/100ms	10.227k i/100ms
with a multi-part traversal path including the base_directory	5.848k i/100ms	40.974k i/100ms	43.127k i/100ms	43.091k i/100ms
with no questionable path	70.152k (± 1.9%) i/s - 355.827k in 5.074255s	674.568k (± 2.8%) i/s - 3.402M in 5.047108s	740.184k (± 1.4%) i/s - 3.749M in 5.066490s	753.797k (± 1.6%) i/s - 3.779M in 5.014342s
with a single-part questionable path	66.758k (± 2.7%) i/s - 337.620k in 5.061671s	656.353k (± 4.1%) i/s - 3.327M in 5.079440s	731.445k (± 0.7%) i/s - 3.658M in 5.002016s	741.240k (± 3.5%) i/s - 3.727M in 5.035737s
with a multi-part questionable path	63.044k (± 4.2%) i/s - 317.500k in 5.046474s	642.866k (± 3.0%) i/s - 3.235M in 5.037611s	686.939k (±12.2%) i/s - 3.385M in 5.055924s	719.741k (± 2.1%) i/s - 3.646M in 5.068564s
with a single-part traversal path	66.523k (± 2.6%) i/s - 333.744k in 5.020730s	655.010k (± 2.8%) i/s - 3.297M in 5.037056s	722.345k (± 1.4%) i/s - 3.612M in 5.001173s	721.008k (± 1.3%) i/s - 3.625M in 5.029201s
with a multi-part traversal path	59.564k (±17.5%) i/s - 290.444k in 5.133958s	618.577k (± 2.9%) i/s - 3.153M in 5.101243s	659.161k (± 5.8%) i/s - 3.315M in 5.049436s	688.935k (± 1.4%) i/s - 3.472M in 5.040412s
with the exact same paths	3.384M (±19.7%) i/s - 16.507M in 5.128966s	3.771M (± 9.5%) i/s - 18.923M in 5.070652s	3.821M (± 2.8%) i/s - 19.200M in 5.028773s	4.104M (± 1.4%) i/s - 20.628M in 5.027120s
with a single-part absolute path including the base_directory	52.928k (±20.3%) i/s - 252.928k in 5.009928s	285.799k (± 3.5%) i/s - 1.446M in 5.066518s	300.236k (± 1.4%) i/s - 1.503M in 5.006624s	305.174k (± 1.4%) i/s - 1.529M in 5.012260s
with a multi-part absolute path including the base_directory	57.189k (±21.4%) i/s - 277.049k in 5.216751s	282.806k (± 5.1%) i/s - 1.438M in 5.101322s	281.253k (± 1.5%) i/s - 1.431M in 5.088109s	297.239k (± 7.3%) i/s - 1.503M in 5.091664s
with a single-part traversal path including the base_directory	82.299k (±22.6%) i/s - 391.251k in 5.068463s	97.885k (± 1.6%) i/s - 498.300k in 5.092095s	101.875k (± 1.8%) i/s - 511.160k in 5.019353s	95.934k (±11.3%) i/s - 480.669k in 5.100200s
with a multi-part traversal path including the base_directory	37.547k (±12.6%) i/s - 187.136k in 5.087265s	400.997k (± 2.5%) i/s - 2.008M in 5.010275s	429.309k (± 1.7%) i/s - 2.156M in 5.024406s	432.577k (± 1.8%) i/s - 2.198M in 5.082193s

[fauno]

The reason I'm saying this is not to discourage you but to err on the safer side. In the past, I myself had used instance variables as cache in a module (see Jekyll::Filters#where). Luckily, only one user had problems because of it (to my knowledge).

I understand! But I can't think of a solution :)

[ashmaroli]

Using Jekyll::PathManager.join has exposed situations where the result of Jekyll.sanitized_path could be mutated..
(See CI logs for details.)

[fauno]

Fixed!

[ashmaroli]

Nice! Do you see a way to move the implementation into Jekyll::PathManager itself?
That would remove my concerns about instance_variables in the top-level module..

def sanitized_path(base, input)
  Jekyll::PathManager.sanitized_path(base, input).dup
end

[fauno]

Sure! It's benchmarking worse but still a lot better than current.

benchmark	without patch	patch + dup + pathmanager	moved to pathmanager
with no questionable path	6.977k i/100ms	75.574k i/100ms	42.284k i/100ms
with a single-part questionable path	6.620k i/100ms	74.534k i/100ms	49.422k i/100ms
with a multi-part questionable path	6.350k i/100ms	70.119k i/100ms	48.458k i/100ms
with a single-part traversal path	6.544k i/100ms	72.508k i/100ms	49.412k i/100ms
with a multi-part traversal path	6.314k i/100ms	68.073k i/100ms	45.977k i/100ms
with the exact same paths	402.619k i/100ms	412.564k i/100ms	123.134k i/100ms
with a single-part absolute path including the base_directory	6.656k i/100ms	30.586k i/100ms	23.442k i/100ms
with a multi-part absolute path including the base_directory	6.443k i/100ms	30.069k i/100ms	23.020k i/100ms
with a single-part traversal path including the base_directory	12.621k i/100ms	10.227k i/100ms	9.003k i/100ms
with a multi-part traversal path including the base_directory	5.848k i/100ms	43.091k i/100ms	30.500k i/100ms
with no questionable path	70.152k (± 1.9%) i/s - 355.827k in 5.074255s	753.797k (± 1.6%) i/s - 3.779M in 5.014342s	488.882k (± 3.3%) i/s - 2.452M in 5.022942s
with a single-part questionable path	66.758k (± 2.7%) i/s - 337.620k in 5.061671s	741.240k (± 3.5%) i/s - 3.727M in 5.035737s	490.041k (± 2.1%) i/s - 2.471M in 5.044907s
with a multi-part questionable path	63.044k (± 4.2%) i/s - 317.500k in 5.046474s	719.741k (± 2.1%) i/s - 3.646M in 5.068564s	486.214k (± 2.5%) i/s - 2.471M in 5.086345s
with a single-part traversal path	66.523k (± 2.6%) i/s - 333.744k in 5.020730s	721.008k (± 1.3%) i/s - 3.625M in 5.029201s	490.451k (± 1.2%) i/s - 2.471M in 5.038176s
with a multi-part traversal path	59.564k (±17.5%) i/s - 290.444k in 5.133958s	688.935k (± 1.4%) i/s - 3.472M in 5.040412s	467.746k (± 1.4%) i/s - 2.345M in 5.014031s
with the exact same paths	3.384M (±19.7%) i/s - 16.507M in 5.128966s	4.104M (± 1.4%) i/s - 20.628M in 5.027120s	1.363M (± 3.0%) i/s - 6.896M in 5.064485s
with a single-part absolute path including the base_directory	52.928k (±20.3%) i/s - 252.928k in 5.009928s	305.174k (± 1.4%) i/s - 1.529M in 5.012260s	223.064k (± 7.2%) i/s - 1.125M in 5.076355s
with a multi-part absolute path including the base_directory	57.189k (±21.4%) i/s - 277.049k in 5.216751s	297.239k (± 7.3%) i/s - 1.503M in 5.091664s	227.993k (± 1.5%) i/s - 1.151M in 5.049620s
with a single-part traversal path including the base_directory	82.299k (±22.6%) i/s - 391.251k in 5.068463s	95.934k (±11.3%) i/s - 480.669k in 5.100200s	93.307k (± 1.6%) i/s - 468.156k in 5.018722s
with a multi-part traversal path including the base_directory	37.547k (±12.6%) i/s - 187.136k in 5.087265s	432.577k (± 1.8%) i/s - 2.198M in 5.082193s	305.594k (± 5.4%) i/s - 1.525M in 5.008005s

[ashmaroli]

The implementation is looking great, @fauno! Nice work!
I'd like to make some more suggestions..

• As of current master, Jekyll.sanitized_path returns the input base under two scenarios. So if the end-usage mutates the result, the given input string should continue to mutate at the end-user-level as expected. Therefore, I suggest moving the first two guard clauses back to the original place:

  # lib/jekyll.rb
  
  def sanitized_path(base_directory, questionable_path)
    return base_directory if base_directory.eql?(questionable_path)
    return base_directory if questionable_path.nil?
  
    Jekyll::PathManager.sanitized_path(base_directory, questionable_path).dup
  end

  That would also ensure unnecessary duplication and also that the result from Jekyll::PathManager.sanitized_path is always frozen as documented.
• The result of return clean_path if clean_path.eql?(base_directory) should also be frozen, even if it isn't cached, for consistency.
• If you'are able to add tests for the Jekyll::PathManager.sanitized_path, well and good. Otherwise, I'll add them to this branch before approval.
• Lastly, a personal nit: I'd like the private_class_method :sanitized_path_cache directive to be flush with the definition:
• Lastly, you can reduce method lookup by use the cache directly:

  # lib/jekyll/path_manager.rb
  
  def self.sanitized_path(base_directory, questionable_path)
    @sanitized_path ||= {}
    @sanitized_path[base_directory] ||= {}
    @sanitized_path[base_directory][questionable_path] ||= begin
      # logic
    end
  end

  (The ABC size got reduced by moving the first two guard clauses out.)

[fauno]

Done! Not sure if that's what you were thinking about tests (too early in the morning over here :)

[ashmaroli]

Not sure if that's what you were thinking about tests

Close.. But not exactly..
Was hoping for a more fleshed test coverage. Its hard to describe because the core logic is already tested via test/test_path_sanitization.rb..

I'll push a couple of commits some time tomorrow..

[ashmaroli]

@fauno I'm satisfied with the current diff.
Let me know if your stackprof / benchmark/ips tests show a regression from your previous tests.

[fauno]

The benchmark looks good! Thanks 👍

[DirtyF]

👏 Great work @fauno, thanks for your help making Jekyll a little bit faster

[DirtyF]

@jekyll: merge +minor

[ashmaroli]

@fauno Thank you for improving Jekyll.
If you don't mind could you share the stackprof results for the site you referred to in the opening comment..?

[fauno]

@fauno Thank you for improving Jekyll.

I'm glad! Thanks for reviewing the patch and maintaining Jekyll :)

If you don't mind could you share the stackprof results for the site you referred to in the opening comment..?

Sure, it's very simple, since I'm just learning to use stackprof. I've put the results on a pastebin since it's 2000 lines long

require 'stackprof'

profile = StackProf.run mode: :object do
  require 'jekyll'

  config = Jekyll.configuration('source' => Dir.pwd,
                                'quiet' => true,
                                'watch' => false,
                                'safe' => true)

  config.delete('theme')
  config['markdown'] = 'kramdown'
  config['plugins'] = []

  site = Jekyll::Site.new(config)
  site.process
end

StackProf::Report.new(profile).print_text

[ashmaroli]

Thanks for including the script as well. However, this repository already has a script to stackprof during dev:
https://github.com/jekyll/jekyll/blob/master/script/stackprof