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

Initialize include-files as Jekyll objects #8158

Merged
merged 15 commits into from May 25, 2020

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented May 4, 2020

Summary

This PR optimizes the processing of include files.

Currently, include files are located and read into memory for each call to the tag's render method. Though the parsed include templates are cached, they're cached to a given Liquid context. So, the file warrants a re-parsing when the context changes.

With the proposed changes, include files are initialized into new Jekyll::Inclusion core objects. The actual file-contents and its parsing are deferred until necessary (during the render calls) and subsequently cached until next regeneration.
To maintain backwards-compatibility with Ruby API, the rendering is now handled by a new (temporary) subclass of Tags::IncludeTag.

Does not need documentation updates or tests since there are no discernible changes to either Ruby API or API exposed for Liquid templates.

@DirtyF
Copy link
Member

DirtyF commented May 4, 2020

Manual testing does not show any build time improvement on our docs.

@ashmaroli ashmaroli marked this pull request as draft May 4, 2020 18:19
@ashmaroli ashmaroli marked this pull request as ready for review May 6, 2020 13:51
@ashmaroli
Copy link
Member Author

Memory profile (on AppVeyor)

--- master https://ci.appveyor.com/project/jekyll/jekyll/builds/32641386/job/nvd9jag59n0u2y3b
+++ PR     https://ci.appveyor.com/project/jekyll/jekyll/builds/32685467/job/vexmf42xsrltwa0p

- Total allocated: 357.98 MB (2873621 objects)
- Total retained:  19.76 MB (109571 objects)
+ Total allocated: 351.86 MB (2853141 objects)
+ Total retained:  19.78 MB (109651 objects)

@DirtyF DirtyF requested a review from a team May 6, 2020 15:17
@DirtyF
Copy link
Member

DirtyF commented May 6, 2020

I'll defer to @mattr- if it's worth the "future" breaking change, hard to evaluate the gain from the only memory consumption results on large sites.

@ashmaroli
Copy link
Member Author

ashmaroli commented May 6, 2020

the "future" breaking change

Technically, it need not progress into a breaking change if we or the maintainers at the time don't want to, but I placed the comment to deter plugin authors and for the maintainers in the future to have a plausible less-messy option.

mattr-
mattr- previously requested changes May 12, 2020
Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

Related to breaking changes: As long as we're not modifying the base contract of the API, then there's no reason we can't make wholesale changes under the hood, as long as the same API works between versions. I don't see anything to worry about with this PR, unless I've misunderstood the issue somehow.

lib/jekyll/inclusion.rb Outdated Show resolved Hide resolved
lib/jekyll/inclusion.rb Outdated Show resolved Hide resolved
lib/jekyll/inclusion.rb Outdated Show resolved Hide resolved
lib/jekyll/inclusion.rb Outdated Show resolved Hide resolved
@ashmaroli
Copy link
Member Author

@mattr- I've dropped the idea of using a relative_path for these instances entirely.

@ashmaroli ashmaroli requested a review from mattr- May 14, 2020 15:41
@ashmaroli ashmaroli requested a review from DirtyF May 25, 2020 13:25
@ashmaroli ashmaroli removed the request for review from mattr- May 25, 2020 13:25
@ashmaroli ashmaroli dismissed mattr-’s stale review May 25, 2020 13:25

Concerns addressed..

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

Notable gain if we compare current build time with master.

@DirtyF DirtyF added this to the 4.1 milestone May 25, 2020
@ashmaroli
Copy link
Member Author

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 1292dcc into jekyll:master May 25, 2020
@jekyllbot jekyllbot added the fix label May 25, 2020
jekyllbot added a commit that referenced this pull request May 25, 2020
@ashmaroli ashmaroli deleted the inclusion-objects branch May 25, 2020 17:01
@jekyll jekyll locked and limited conversation to collaborators May 25, 2021
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.

None yet

5 participants