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

Stash frequently used Drop setter keys for reuse #8394

Merged
merged 4 commits into from
Oct 11, 2020

Conversation

ashmaroli
Copy link
Member

  • This is an ⚡ optimization change.

Summary

Jekyll::Render is initialized for every page or document rendered in a site and invariably calls Jekyll::Drops::Drop#[]= with a series of keys via its #run method:

def run
Jekyll.logger.debug "Rendering:", document.relative_path
assign_pages!
assign_current_document!
assign_highlighter_options!
assign_layout_data!


def assign_pages!
payload["page"] = document.to_liquid
payload["paginator"] = (document.pager.to_liquid if document.respond_to?(:pager))
end

def assign_highlighter_options!
payload["highlighter_prefix"] = converters.first.highlighter_prefix
payload["highlighter_suffix"] = converters.first.highlighter_suffix
end

def assign_layout_data!
layout = layouts[document.data["layout"]]
payload["layout"] = Utils.deep_merge_hashes(layout.data, payload["layout"] || {}) if layout
end

def render_layout(output, layout, info)
payload["content"] = output
payload["layout"] = Utils.deep_merge_hashes(layout.data, payload["layout"] || {})


The constant is currently private so that it can be replaced with a class-scoped cache in a future version if needed.

@ashmaroli
Copy link
Member Author

ashmaroli commented Sep 17, 2020

Third Party Repo Profile Summary

--- master
+++ PR

- Total allocated: 282.43 MB (2467827 objects)
+ Total allocated: 280.79 MB (2426843 objects)
  Total retained:  37.81 MB (112069 objects)

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.

No gain on the build time?

@ashmaroli
Copy link
Member Author

No gain on the build time?

No. Because we're replacing String interpolation with a Hash lookup.
At interpreter level, they're comparably performant.

The notable gain here is reduced allocations in a large site with numerous pages and documents especially because Jekyll::Renderer#run will inevitably run for all of them..

@DirtyF
Copy link
Member

DirtyF commented Sep 17, 2020

What is a large site? I thought the one was big enough to reflect gain.

@ashmaroli
Copy link
Member Author

What is a large site?

A site with numerous pages and documents (and excerpts)

I thought the one was big enough to reflect gain.

Yes. The third-party site is large enough.
But the proposed change in the PR does not result in greater performance.
Just reduced allocations and therefore reduced strain on Ruby's Garbage Collector.

@ashmaroli ashmaroli added the memory-optimization ⚡ Reduced memory usage for the work done label Sep 18, 2020
@jekyll jekyll deleted a comment from RamboCacing666 Sep 21, 2020
@jekyll jekyll deleted a comment from RamboCacing666 Sep 21, 2020
@ashmaroli ashmaroli added this to the 4.2 milestone Sep 25, 2020
@ashmaroli
Copy link
Member Author

@jekyllbot: merge +fix

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fix frozen-due-to-age memory-optimization ⚡ Reduced memory usage for the work done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants