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

performance improvements #519

Closed
wants to merge 3 commits into from
Closed

Conversation

lucagrulla
Copy link
Contributor

I did some profiling and noticed how jekyll spends a lot of time in doing comparisons, namely Post comparison (via <=>).

Post comparison is actually mostly called from site_payload, that generates a on-demand hash containing (among others) the ordered list of posts. In methods where site_payload is called more than once I just introduced a local variable to reduce the multiple calls and the additional comparison.

Just this in my benchmark gives an improvement in the time needed to generate the site of a 20-25%(particularly useful for big site, with over 1000 between pages and posts).

@tmm1
Copy link
Contributor

tmm1 commented Mar 4, 2012

👍

What version of ruby were you benchmarking with?

@lucagrulla
Copy link
Contributor Author

I am using 1.9.2p290(on a MacBookAir 2.13Ghz Core 2 Duo 4GB RAM).

@@ -183,10 +183,11 @@ def related_posts(posts)
# Returns nothing
def render(layouts, site_payload)
# construct payload
sp = site_payload
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't site_payload already a local variable here? It's passed into the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you re totally right.
my bad.

I'm going to revert this one fix in my fork.
Dp you want a new pull request ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can force push to this pull request's branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
I can actually see my last commit on my fork reverting that code down here in this pull request thread.
I guess it's now part of the pull request then.

@lucagrulla
Copy link
Contributor Author

hey I can see that the pull request has been merged in master (right now it's the last commit). Is then the right time to close it as well ?

@parkr
Copy link
Member

parkr commented Dec 26, 2012

Merged in 7d88f72.

@parkr parkr closed this Dec 26, 2012
@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
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

4 participants