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

Bypass rendering via Liquid unless required #6735

Merged
merged 4 commits into from Feb 28, 2018

Conversation

Projects
None yet
9 participants
@ashmaroli
Member

ashmaroli commented Feb 1, 2018

Currently, all Page objects and certain Document objects are always parsed and rendered via Liquid before being passed onto the concerned Converter.

With this patch, a simple check is performed after :pre-render hooks to determine if the content contains any Liquid Tags / Variables and then handled accordingly:

  • if the content is simply 100% plaintext (or Markdown), then Liquid's parse-render phase is skipped and directly passed onto the associated Converter
  • otherwise, follow current behavior

/cc @jekyll/build

@jekyllbot jekyllbot requested review from ayastreb, mattr- and parkr Feb 1, 2018

@ashmaroli ashmaroli removed the request for review from ayastreb Feb 1, 2018

@pathawks

This comment has been minimized.

Member

pathawks commented Feb 1, 2018

How expensive are the two .include? operations plus an .empty? check, vs just letting Liquid perform a noop?

Would really like to see some benchmarks here.

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 1, 2018

@pathawks tested it on front-matter-default repo and no significant change (more or less the same)

@pathawks

This comment has been minimized.

Member

pathawks commented Feb 1, 2018

I would expect that Liquid’s parsing is no more expensive (and possibly less) than a couple RegExes when the content contains no Liquid tags. The extra RegExes will (of course) make rendering take longer when documents do contain Liquid markup.

Because this introduces complexity, I am hesitant to accept unless the benefits are pretty substantial.

# Returns true is the string contains sequences of `{%` or `{{`
def has_liquid_construct?(content)
return false if content.nil? || content.empty?
content.include?("{%") || content.include?("{{")

This comment has been minimized.

@pathawks

pathawks Feb 1, 2018

Member

Are the strings converted to RegEx on the backend? This might be faster?

!%r!{[{%]!.match(content).nil?

This comment has been minimized.

@ashmaroli

ashmaroli Feb 2, 2018

Member

This might be faster.. or may not.. Needs to be benchmarked.. 😉

@DirtyF DirtyF requested a review from oe Feb 2, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 2, 2018

@pathawks I've added a rudimentary benchmark to assess the proposed change..

@mattr-

This comment has been minimized.

Member

mattr- commented Feb 2, 2018

How big is the front-matter defaults repo? I ask because we'll need to be careful about using microbenchmarks with some of these rendering performance changes as they may introduce real speedups but we won't see it until the size of the site reaches a certain point.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 2, 2018

@mattr- The front-matter-defaults repo doesn't factor in this PR because this PR deals with the rendering difference between: pages/documents that contain Liquid tags/variables vs pages/documents that are purely Markdown or other type of text..

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 2, 2018

I made 10 duplicates of files in the _docs folder of our documentation and ran a single test to check for significant changes: 120s with master vs 118s with this branch. More or less the same given build times can differ.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 2, 2018

10 duplicates of files in the _docs folder

duplicates of jekyll/docs/_docs?

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 2, 2018

Nevermind, I'll let you do your own benchmarks, I just wish we could test these kind of optimizations against real Jekyll large websites.

@pathawks

This comment has been minimized.

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 2, 2018

yeah i thought about that, but none of the websites listed in acceptance tests is very large.

@oe

oe approved these changes Feb 4, 2018

I think this is fundamentally a good idea, and if it saves ~2 seconds with a fairly big site and doesn't break anything, it's fine by me. Would love to see more benchmarks performed on large sites though.

@parkr

This comment has been minimized.

Member

parkr commented Feb 4, 2018

Do you have the output on these benchmarks? Curious to see the output but away from the computer :)

@ashmaroli ashmaroli referenced this pull request Feb 17, 2018

Closed

Release 3.8.0 #6783

12 of 13 tasks complete
@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 17, 2018

@tomjoht blog takes more than 22min to build at that time, we should check all the latest small perf improvements against his website: https://github.com/tomjoht/tomjoht.github.io

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 21, 2018

$ be ruby benchmark/conditional-liquid.rb
Warming up --------------------------------------
         regex-check    71.427k i/100ms
       builtin-check   152.025k i/100ms
Calculating -------------------------------------
         regex-check    961.880k (± 3.5%) i/s -      4.857M in   5.056289s
       builtin-check      3.308M (± 5.1%) i/s -     16.571M in   5.026962s

Comparison:
       builtin-check:  3307537.3 i/s
         regex-check:   961880.1 i/s - 3.44x  slower

Warming up --------------------------------------
         regex-check    55.609k i/100ms
       builtin-check   125.621k i/100ms
Calculating -------------------------------------
         regex-check    691.777k (± 7.6%) i/s -      3.448M in   5.022451s
       builtin-check      2.137M (± 3.3%) i/s -     10.678M in   5.003542s

Comparison:
       builtin-check:  2136608.2 i/s
         regex-check:   691776.7 i/s - 3.09x  slower

Warming up --------------------------------------
         regex-check    53.448k i/100ms
       builtin-check    94.997k i/100ms
Calculating -------------------------------------
         regex-check    617.459k (±12.0%) i/s -      3.047M in   5.017810s
       builtin-check      1.376M (± 5.8%) i/s -      6.935M in   5.059654s

Comparison:
       builtin-check:  1376031.7 i/s
         regex-check:   617459.4 i/s - 2.23x  slower

Warming up --------------------------------------
always thru liquid - plain text
                         4.812k i/100ms
conditional liquid - plain text
                       150.590k i/100ms
Calculating -------------------------------------
always thru liquid - plain text
                         49.822k (± 3.1%) i/s -    250.224k in   5.027488s
conditional liquid - plain text
                          2.931M (± 7.1%) i/s -     14.607M in   5.016229s

Comparison:
conditional liquid - plain text:  2931073.6 i/s
always thru liquid - plain text:    49822.0 i/s - 58.83x  slower

Warming up --------------------------------------
always thru liquid - tags n vars
                       785.000  i/100ms
conditional liquid - tags n vars
                       765.000  i/100ms
Calculating -------------------------------------
always thru liquid - tags n vars
                          8.354k (± 6.1%) i/s -     41.605k in   5.000493s
conditional liquid - tags n vars
                          8.394k (± 5.7%) i/s -     42.075k in   5.030741s

Comparison:
conditional liquid - tags n vars:     8394.2 i/s
always thru liquid - tags n vars:     8354.5 i/s - same-ish: difference falls within error

Warming up --------------------------------------
always thru liquid - just vars
                         1.259k i/100ms
conditional liquid - just vars
                         1.234k i/100ms
Calculating -------------------------------------
always thru liquid - just vars
                         12.765k (± 5.5%) i/s -     64.209k in   5.048326s
conditional liquid - just vars
                         12.867k (± 2.9%) i/s -     65.402k in   5.087556s

Comparison:
conditional liquid - just vars:    12867.0 i/s
always thru liquid - just vars:    12764.9 i/s - same-ish: difference falls within error
@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 21, 2018

Filtering above result to show the most relevant data for this PR:

Warming up --------------------------------------
always thru liquid - plain text
                         4.812k i/100ms
conditional liquid - plain text
                       150.590k i/100ms
Calculating -------------------------------------
always thru liquid - plain text
                         49.822k (± 3.1%) i/s -    250.224k in   5.027488s
conditional liquid - plain text
                          2.931M (± 7.1%) i/s -     14.607M in   5.016229s

Comparison:
conditional liquid - plain text:  2931073.6 i/s
always thru liquid - plain text:    49822.0 i/s - 58.83x  slower

To summarize, if there were a site with lots of Markdown files without any Liquid content whatsoever, then it its faster to pass the string text to the converter directly instead of passing the string text through Liquid's Parser first.

@pathawks

This comment has been minimized.

Member

pathawks commented Feb 21, 2018

I changed check_with_regex to use String.match? instead of Regexp.match so that it will stop looking after finding a single match. This is now faster than check_with_builtin if the input contains any Liquid.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 21, 2018

@pathawks Two questions:

  • can you post the updated benchmark results?
  • can I split the benchmarks into two separate files?
@pathawks

This comment has been minimized.

Member

pathawks commented Feb 21, 2018

can I split the benchmarks into two separate files?

Sure 👍

Warming up --------------------------------------
regex-check   - plain text
                       176.614k i/100ms
builtin-check - plain text
                       229.229k i/100ms
Calculating -------------------------------------
regex-check   - plain text
                          2.897M (± 2.3%) i/s -     14.659M in   5.062371s
builtin-check - plain text
                          4.784M (± 3.9%) i/s -     24.069M in   5.039610s

Comparison:
builtin-check - plain text:  4783868.4 i/s
regex-check   - plain text:  2897317.0 i/s - 1.65x  slower

Warming up --------------------------------------
regex-check   - tags n vars
                       214.528k i/100ms
builtin-check - tags n vars
                       180.318k i/100ms
Calculating -------------------------------------
regex-check   - tags n vars
                          4.204M (± 3.0%) i/s -     21.024M in   5.006021s
builtin-check - tags n vars
                          3.057M (± 2.0%) i/s -     15.327M in   5.016518s

Comparison:
regex-check   - tags n vars:  4203840.7 i/s
builtin-check - tags n vars:  3056602.0 i/s - 1.38x  slower

Warming up --------------------------------------
regex-check   - just vars
                       198.649k i/100ms
builtin-check - just vars
                       134.002k i/100ms
Calculating -------------------------------------
regex-check   - just vars
                          3.649M (± 3.6%) i/s -     18.276M in   5.015865s
builtin-check - just vars
                          1.952M (± 3.3%) i/s -      9.782M in   5.017950s

Comparison:
regex-check   - just vars:  3648780.1 i/s
builtin-check - just vars:  1951683.0 i/s - 1.87x  slower
@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 21, 2018

hmm.. I'm in a fix now.. not sure what would be the best way to proceed..

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 21, 2018

@pathawks just realized that String#match? was introduced only in Ruby 2.4.0.. So we can't use it now.. 😁

@pathawks

This comment has been minimized.

Member

pathawks commented Feb 21, 2018

String#match? was introduced only in Ruby 2.4.0

🤣 That makes things easy then 👍

@DirtyF

DirtyF approved these changes Feb 21, 2018

Benchmark, green tests 💚

@ashmaroli ashmaroli added this to the v3.8.0 milestone Feb 21, 2018

@Crunch09

👍

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 28, 2018

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 23bb50c into jekyll:master Feb 28, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ashmaroli ashmaroli deleted the ashmaroli:bypass-liquid-to-converter branch Feb 28, 2018

@pathawks

This comment has been minimized.

Member

pathawks commented Apr 30, 2018

The jekyll-feed test suite is broken because of this PR.

The failing test: it "renders Liquid inside posts"

@pathawks

This comment has been minimized.

Member

pathawks commented Apr 30, 2018

Turns out, this is already fixed by #6945 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment