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..

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:

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.

Looking good :)

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 :)

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

Fixed!

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

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

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.)

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..

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

The benchmark looks good! Thanks 👍

@jekyll: merge +minor

@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 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

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