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

Cache include file to save liquid parsing time. #4120

Merged
merged 1 commit into from Nov 18, 2015

Conversation

Projects
None yet
5 participants
@rebornix
Member

rebornix commented Nov 9, 2015

This is mainly inspired by Shopify Liquid include tag, reducing redundant include file parsing time.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 9, 2015

Contributor

I like it, thanks @rebornix!

Contributor

envygeeks commented Nov 9, 2015

I like it, thanks @rebornix!

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 9, 2015

Member

@envygeeks Your suggestion is awesome! This function turns out to be a simple if/else block

Member

rebornix commented Nov 9, 2015

@envygeeks Your suggestion is awesome! This function turns out to be a simple if/else block

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 13, 2015

Member

Guys, I just gather some stats for this enhancement. To make the stats data more intuitive, I separate parsing time from total rendering time when build site with --profile option.

Take Jekyll official docs for example, before caching include files, includes are parsed hundred times and part of them take about 100ms.

image

After this patch is applied, include files parsing time decrease significantly like below,

image

Even though the total build time of Jekyll docs only decrease about 5%, but sites heavily using _includes in their layout may benefit a lot 😃

Member

rebornix commented Nov 13, 2015

Guys, I just gather some stats for this enhancement. To make the stats data more intuitive, I separate parsing time from total rendering time when build site with --profile option.

Take Jekyll official docs for example, before caching include files, includes are parsed hundred times and part of them take about 100ms.

image

After this patch is applied, include files parsing time decrease significantly like below,

image

Even though the total build time of Jekyll docs only decrease about 5%, but sites heavily using _includes in their layout may benefit a lot 😃

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 13, 2015

Contributor

I'm 👍 one this, just waiting for others to review.

Contributor

envygeeks commented Nov 13, 2015

I'm 👍 one this, just waiting for others to review.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 18, 2015

Contributor

/cc @jekyll/core

Contributor

envygeeks commented Nov 18, 2015

/cc @jekyll/core

@envygeeks envygeeks added this to the 3.1 milestone Nov 18, 2015

parkr added a commit that referenced this pull request Nov 18, 2015

@parkr parkr merged commit c1761bc into jekyll:master Nov 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Nov 18, 2015

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 18, 2015

Member

Thanks!

Member

parkr commented Nov 18, 2015

Thanks!

@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Nov 19, 2015

Contributor

Spotted an issue using this when combined with jekyll-assets (and perhaps other plugins/examples where liquid is parsed within liquid). Example, when I use the following code:

{% for item in items %}
    {{ item.title | slugify }}
    {% asset_source "c-item__logo--{{ item.title | slugify }}.svg" %}
{% endfor %}

The following is produced:

24_ways
<svg class="24ways" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

unicef_uk
<svg class="24ways" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

bradshaw_s_guide
<svg class="24ways" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

(the generated asset is the same asset). For comparison, the desired output should be:

24_ways
<svg class="24ways" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

unicef_uk
<svg class="unicef" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

bradshaw_s_guide
<svg class="bradshaws" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

Should I now file this as a separate bug? In Jekyll, or Jekyll Assets?

Contributor

paulrobertlloyd commented Nov 19, 2015

Spotted an issue using this when combined with jekyll-assets (and perhaps other plugins/examples where liquid is parsed within liquid). Example, when I use the following code:

{% for item in items %}
    {{ item.title | slugify }}
    {% asset_source "c-item__logo--{{ item.title | slugify }}.svg" %}
{% endfor %}

The following is produced:

24_ways
<svg class="24ways" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

unicef_uk
<svg class="24ways" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

bradshaw_s_guide
<svg class="24ways" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

(the generated asset is the same asset). For comparison, the desired output should be:

24_ways
<svg class="24ways" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

unicef_uk
<svg class="unicef" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

bradshaw_s_guide
<svg class="bradshaws" xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 960 960">... </svg>

Should I now file this as a separate bug? In Jekyll, or Jekyll Assets?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 19, 2015

Member

Parsing and rendering should not interact in this way... @envygeeks how does asset_source work?

Member

parkr commented Nov 19, 2015

Parsing and rendering should not interact in this way... @envygeeks how does asset_source work?

@rebornix rebornix deleted the rebornix:CacheIncludeTemplate branch Nov 19, 2015

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 19, 2015

Member
{{ item.title | slugify }}

works as charm, but assets_source didn't handle inside liquid rendering correctly. Seems it's related to issue Parse Liquid output within asset_source tag.

Member

rebornix commented Nov 19, 2015

{{ item.title | slugify }}

works as charm, but assets_source didn't handle inside liquid rendering correctly. Seems it's related to issue Parse Liquid output within asset_source tag.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 19, 2015

Contributor

@rebornix before you go blaming me explain yourself. We handle rendering directly through Jekyll: https://github.com/jekyll/jekyll-assets/blob/master/lib/jekyll/assets/liquid/tag/parser.rb#L172 so I expect you to be very careful and explicit about how it's my fault especially if this is happening after your pull is merged.

Contributor

envygeeks commented Nov 19, 2015

@rebornix before you go blaming me explain yourself. We handle rendering directly through Jekyll: https://github.com/jekyll/jekyll-assets/blob/master/lib/jekyll/assets/liquid/tag/parser.rb#L172 so I expect you to be very careful and explicit about how it's my fault especially if this is happening after your pull is merged.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 19, 2015

Contributor

@parkr @rebornix @paulrobertlloyd we take in the source they give us, we parse as normal and then we run the value through Jekyll's own renderer with the context (and only the context) the user is in so they get those variables. We use the raw args as the filename so that if a user error pops up they get the raw args spot as the place the error happened.

Contributor

envygeeks commented Nov 19, 2015

@parkr @rebornix @paulrobertlloyd we take in the source they give us, we parse as normal and then we run the value through Jekyll's own renderer with the context (and only the context) the user is in so they get those variables. We use the raw args as the filename so that if a user error pops up they get the raw args spot as the place the error happened.

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 19, 2015

Member

@envygeeks It's my mistake to make arbitrary judgement without any solid proof. I apologise.

When I say some code doesn't handle some logic correctly (my judgement might be correct or wrong), I'm not blaming people who wrote the code. If I make you feel that way, I apologise.

Let me take a deep look into this issue and give it a turnaround. Sorry for bringing you bad feelings, sincerely.

Member

rebornix commented Nov 19, 2015

@envygeeks It's my mistake to make arbitrary judgement without any solid proof. I apologise.

When I say some code doesn't handle some logic correctly (my judgement might be correct or wrong), I'm not blaming people who wrote the code. If I make you feel that way, I apologise.

Let me take a deep look into this issue and give it a turnaround. Sorry for bringing you bad feelings, sincerely.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 19, 2015

Contributor

@rebornix I'm already looking into it, and I don't know if it's either one of our faults. I'm using the context being passed to me to render but after doing a deep test pre-your commit I don't ever get an updated context so I always end up with the same n for some reason.

Contributor

envygeeks commented Nov 19, 2015

@rebornix I'm already looking into it, and I don't know if it's either one of our faults. I'm using the context being passed to me to render but after doing a deep test pre-your commit I don't ever get an updated context so I always end up with the same n for some reason.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 19, 2015

Contributor

This is my bug, I don't know where but for some reason our contexts are being called dozens of times and it leads to a race. I don't know how that is happening since we only hit the caller once but I'll have to track down where.

Contributor

envygeeks commented Nov 19, 2015

This is my bug, I don't know where but for some reason our contexts are being called dozens of times and it leads to a race. I don't know how that is happening since we only hit the caller once but I'll have to track down where.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 19, 2015

Contributor

I take that back I don't know where the bug is. I don't even get it. I'm gonna need a few hours.

Contributor

envygeeks commented Nov 19, 2015

I take that back I don't know where the bug is. I don't even get it. I'm gonna need a few hours.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 19, 2015

Contributor

I know the problem. It's because we expect tag to be called for every tag but it's actually called multiple times with multiple contexts and our own caching and destructive parsing is pushing out the new context. I had no idea Liquid works this way so I'll need to rework our parser to be less desctructive with our hashes on the assumption we are a fire and reload fire instead of a fire fire fire.

Contributor

envygeeks commented Nov 19, 2015

I know the problem. It's because we expect tag to be called for every tag but it's actually called multiple times with multiple contexts and our own caching and destructive parsing is pushing out the new context. I had no idea Liquid works this way so I'll need to rework our parser to be less desctructive with our hashes on the assumption we are a fire and reload fire instead of a fire fire fire.

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 19, 2015

Member

@envygeeks Great to hear you find the root cause 👍 😄

Putting more info here for this PR: It caches include parsed templates for saving liquid parsing time and that's all. If you feel this pr may lead to some bugs, a quick way to filter it out is to create a layout without any include block then test again.

Member

rebornix commented Nov 19, 2015

@envygeeks Great to hear you find the root cause 👍 😄

Putting more info here for this PR: It caches include parsed templates for saving liquid parsing time and that's all. If you feel this pr may lead to some bugs, a quick way to filter it out is to create a layout without any include block then test again.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks
Contributor

envygeeks commented Nov 19, 2015

@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.