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 documents write? attribute in a variable #8389

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

ashmaroli
Copy link
Member

  • This is an ⚡ optimization change.

Summary

Instead of checking collection&.write? && site.publisher.publish?(self) on every call to the :write? method, compute the result once per instance and reuse the result on subsequent calls.

Context

document.write? is invoked via various core classes (e.g. Site, Regenerator) and in-house plugins (e.g. jekyll-mentions, jemoji)

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.

Good catch 🥇

@ashmaroli
Copy link
Member Author

Thanks :)
@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit bd04997 into jekyll:master Sep 14, 2020
@ashmaroli ashmaroli deleted the stash-doc-write-attribute branch September 14, 2020 13:38
jekyllbot added a commit that referenced this pull request Sep 14, 2020
@ashmaroli ashmaroli added this to the 4.2 milestone Sep 14, 2020
@DirtyF
Copy link
Member

DirtyF commented Sep 14, 2020

@ashmaroli I hadn't build Jekyll website locally for a few months, last time it was taking 6.2s on my machine, current master is now at 3.6s. 🚀 👏

@ashmaroli
Copy link
Member Author

The recent optimizations aren't major ones. Just minor bottlenecks I discovered when profiling various sites in the wild.
Our docs site is too small to get a noticeable improvement from these changes..
Thanks for the encouragement anyways. 😃
Cheers 🥂

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

3 participants