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 allocations from URLFilters #7641

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 8 additions & 10 deletions lib/jekyll/filters/url_filters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@ def absolute_url(input)
return if input.nil?

input = input.url if input.respond_to?(:url)
return input if Addressable::URI.parse(input.to_s).absolute?
input = input.to_s
return input if Addressable::URI.parse(input).absolute?

site = @context.registers[:site]
return relative_url(input) if site.config["url"].nil?

Addressable::URI.parse(
site.config["url"].to_s + relative_url(input)
site.sanitized_url + relative_url(input)
).normalize.to_s
end

Expand All @@ -32,11 +33,13 @@ def relative_url(input)
return if input.nil?

input = input.url if input.respond_to?(:url)
return input if Addressable::URI.parse(input.to_s).absolute?
input = input.to_s
return input if Addressable::URI.parse(input).absolute?

site = @context.registers[:site]

parts = [sanitized_baseurl, input]
Addressable::URI.parse(
parts.compact.map { |part| ensure_leading_slash(part.to_s) }.join
ensure_leading_slash(site.sanitized_baseurl) + ensure_leading_slash(input)
).normalize.to_s
end

Expand All @@ -53,11 +56,6 @@ def strip_index(input)

private

def sanitized_baseurl
site = @context.registers[:site]
site.config["baseurl"].to_s.chomp("/")
end

def ensure_leading_slash(input)
return input if input.nil? || input.empty? || input.start_with?("/")

Expand Down
6 changes: 5 additions & 1 deletion lib/jekyll/site.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

module Jekyll
class Site
attr_reader :source, :dest, :cache_dir, :config
attr_reader :source, :dest, :cache_dir, :config, :sanitized_baseurl, :sanitized_url
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how I feel about adding new public attr_readers to Jekyll::Site, since they'll become part of the public API.

Are there any disadvantages to always returning a sanitized baseurl and a sanitized url?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disadvantage is unnecessary allocations. Technically, I'm only using site.sanitized_baseurl. The sanitized_url is just being provided for completeness.

Since you're on the fence about this, let us wait and see if the optimizations make a difference with a future state of master

attr_accessor :layouts, :pages, :static_files, :drafts,
:exclude, :include, :lsi, :highlighter, :permalink_style,
:time, :future, :unpublished, :safe, :plugins, :limit_posts,
Expand Down Expand Up @@ -50,6 +50,10 @@ def config=(config)
send("#{opt}=", config[opt])
end

# Sanitize here to avoid multiple allocations within our URLFilters.
@sanitized_baseurl = Utils.strip_leading_or_trailing_slashes config["baseurl"]
@sanitized_url = Utils.strip_leading_or_trailing_slashes config["url"]

# keep using `gems` to avoid breaking change
self.gems = config["plugins"]

Expand Down
12 changes: 12 additions & 0 deletions lib/jekyll/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,18 @@ def has_liquid_construct?(content)
end
# rubocop: enable PredicateName

# Returns an empty string if given object is `nil` or `false`.
# Otherwise returns a copy of the given string or the string representation of given
# object with any slashes at the leading end or the trailing end stripped away.
#
# "//blog//" => "blog"
# "blog//" => "blog"
# "//my-domain.com/blog/" => "my-domain.com/blog"
#
def strip_leading_or_trailing_slashes(input)
input ? input.to_s.gsub(%r!^/+|/+$!, "") : ""
end

# Slugify a filename or title.
#
# string - the filename or title to slugify
Expand Down
19 changes: 17 additions & 2 deletions test/test_filters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,21 @@ def select; end
assert_equal "http://example.com/base/#{page_url}", filter.absolute_url(page_url)
end

should "be ok with a 'url' with trailing slash(es)" do
page_url = "about/my_favorite_page/"
filter = make_filter_mock(
"url" => "http://example.com/",
"baseurl" => "/base"
)
assert_equal "http://example.com/base/#{page_url}", filter.absolute_url(page_url)

filter = make_filter_mock(
"url" => "http://example.com//",
"baseurl" => "/base"
)
assert_equal "http://example.com/base/#{page_url}", filter.absolute_url(page_url)
end

should "be ok with a blank but present 'url'" do
page_url = "about/my_favorite_page/"
filter = make_filter_mock(
Expand Down Expand Up @@ -568,13 +583,13 @@ def select; end
assert_equal "/base/css/main.css", filter.relative_url(page_url)
end

should "not return valid URI if baseurl ends with multiple '/'" do
should "return valid URI if baseurl ends with multiple '/'" do
page_url = "/css/main.css"
filter = make_filter_mock(
"url" => "http://example.com",
"baseurl" => "/base//"
)
refute_equal "/base/css/main.css", filter.relative_url(page_url)
assert_equal "/base/css/main.css", filter.relative_url(page_url)
end

should "not prepend a forward slash if both input and baseurl are simply '/'" do
Expand Down
10 changes: 10 additions & 0 deletions test/test_site.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,16 @@ def read_posts
assert_equal "/blog", site.baseurl
end

should "strip leading and trailing slashes from baseurl passed in from config" do
site = Site.new(site_configuration("baseurl" => "//blog//"))
assert_equal "blog", site.sanitized_baseurl
end

should "strip leading and trailing slashes from site_url passed in from config" do
site = Site.new(site_configuration("url" => "//my-domain.com/blog//"))
assert_equal "my-domain.com/blog", site.sanitized_url
end

should "only include theme includes_path if the path exists" do
site = fixture_site("theme" => "test-theme")
assert_equal [source_dir("_includes"), theme_dir("_includes")],
Expand Down