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

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented May 2, 2019

Summary

Many temporary objects are repeatedly allocated within relative_url and absolute_url.
This PR attempts to reduce allocating such temporary objects.

In the following lines:

return input if Addressable::URI.parse(input.to_s).absolute?
parts = [sanitized_baseurl, input]
Addressable::URI.parse(
parts.compact.map { |part| ensure_leading_slash(part.to_s) }.join

  • ln#35 calls input.to_s but does not store the result
  • ln#37 initializes a two-member array on each call to #relative_url
  • ln#39 duplicates above array via non-destructive compact and non-destructive map with each member being subject to to_s calls.

With the proposed changes, the intermediate array objects and the numerous calls to to_s have been eliminated.

API Changes

  • Removed
    • private method URLFilters#sanitized_baseurl removed. Classes mixing-in the Jekyll::Filters module may need to update to using site.baseurl directly.
  • Added
    • site now responds to :sanitized_baseurl and :sanitized_url (getters)
    • Utility method Utils.strip_leading_or_trailing_slashes
  • Changed
    • relative_url and absolute_url now handles multiple leading or trailing slashes in both config["baseurl"] and config["url"]

Context

Additionally resolves #6947

@ashmaroli ashmaroli requested a review from a team May 2, 2019 11:27
@ashmaroli ashmaroli added the breaking-change Affect current behavior label May 2, 2019
@mattr-
Copy link
Member

mattr- commented May 2, 2019

I do not want to introduce another breaking change in the 4.0 release cycle. Let's write a custom baseurl= that does the sanitization for us when the new baseurl is set, keeping us from removing that bit of public API. The rest I'm happy to keep, since folks shouldn't be using private methods anyways.

@ashmaroli ashmaroli added fix and removed breaking-change Affect current behavior labels May 2, 2019
@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/532976771

- Total allocated: 532.65 MB (5301707 objects)
- Total retained:  18.34 MB (96237 objects)
+ Total allocated: 532.22 MB (5295029 objects)
+ Total retained:  18.34 MB (96242 objects)

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

@ashmaroli ashmaroli added this to the 4.1 milestone Jun 14, 2019
@DirtyF DirtyF added this to In progress in Jekyll 4.1 Aug 14, 2019
@DirtyF DirtyF moved this from In progress to Reviewable in Jekyll 4.1 Aug 14, 2019
@DirtyF
Copy link
Member

DirtyF commented Aug 22, 2019

Do we need to revisit this after #7793 ?

@ashmaroli
Copy link
Member Author

Do we need to revisit this after #7793 ?

No. I don't think it's worth the added complexity.
Closing..

@ashmaroli ashmaroli closed this Aug 22, 2019
@ashmaroli ashmaroli deleted the reduce-url-filters-allocations branch August 22, 2019 17:11
@ashmaroli ashmaroli removed this from the 4.1 milestone Aug 22, 2019
@ashmaroli ashmaroli removed this from Reviewable in Jekyll 4.1 Aug 22, 2019
@jekyll jekyll locked and limited conversation to collaborators Aug 21, 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.

Extra forward slash in URLs when config["url"] has a trailing forward slash
4 participants