Reduce string allocations from generating doc URLs #8392
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ashmaroli
commented
Sep 16, 2020
| @@ -94,7 +94,8 @@ def possible_keys(key) | |||
|
|
|||
| def generate_url_from_drop(template) | |||
| template.gsub(%r!:([a-z_]+)!) do |match| | |||
| pool = possible_keys(match.sub(":", "")) | |||
| name = Regexp.last_match(1) | |||
| pool = name.end_with?("_") ? [name, name.chomp!("_")] : [name] | |||
There was a problem hiding this comment.
Notes:
Regexp.last_match(1)here is equivalent tomatch.sub(":", "")but without the extra allocation.- The
#possible_keyspublic method does not mutate the argumentkeybut this inlined version modifies the string in-place.
There was a problem hiding this comment.
Given possible_keys is not used anywhere else, should rewrite or remove it?
There was a problem hiding this comment.
Sadly, It is not a private method. So we'll have to bear the tech debt until v5.0
| match.sub(":#{winner}", replacement) | ||
| end.squeeze("/") | ||
| match.sub!(":#{winner}", replacement) | ||
| end |
There was a problem hiding this comment.
Notes:
- Calling
String#squeezehere is redundant with respect to the bigger picture (Jekyll::URL.new.to_s) because the result from this method is passed ontoJekyll::URL#sanitize_urlthat removes extra"/"from input strings. - The local
matchvariable value is not further required. So it can be mutated.
- Total allocated: 300.38 MB (3084063 objects)
- Total retained: 20.75 MB (113527 objects)
+ Total allocated: 300.31 MB (3082539 objects)
+ Total retained: 20.75 MB (113529 objects)This seems to have an impact on build time: - Docs site build: 6.03 seconds
+ Docs site build: 3.969 seconds |
|
Its at times like these that I wish we could revive Utterson from its ashes.. |
Third Party Repo Profile Summary--- master
+++ PR
- Total allocated: 282.43 MB (2467827 objects)
- Total retained: 37.81 MB (112069 objects)
+ Total allocated: 281.22 MB (2441051 objects)
+ Total retained: 37.81 MB (112070 objects) |
DirtyF
approved these changes
Sep 17, 2020
|
@jekyllbot: merge +fix |
jekyllbot
added a commit
that referenced
this pull request
Sep 18, 2020
This was referenced Mar 12, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Generating URLs from a drop (current route for all documents of a collection) results in a lot of string allocations because of use of non-mutating methods to manipulate strings.
This pull request replaces those non-mutating methods that mutate a duplicated version of long-term strings and mutate temporary strings.