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

Optimize Kramdown::JekyllDocument#to_html calls #8041

Merged
merged 1 commit into from Mar 26, 2020

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Mar 2, 2020

  • This is a 🙋 feature or enhancement.
  • This is tested by existing coverage.

Summary

By having :to_html explicitly defined for the Kramdown::JekyllDocument instances, the benefits are following:

  • Reduce String allocations from converting :to_html to "to_html".
  • Reduce MatchData and String allocations from testing and extracting "html" from the method name and consequent conversion into "Html".
  • Reduce time spent to require "kramdown/converter/html" and consequent constant-lookup.

The optimization is to bypass steps above and call Kramdown::Converter::Html.convert directly. This is possible because Kramdown::Converter::Html is set up to be autoloaded if not available already.

Context

Prior, I made an attempt to propose this patch at the upstream kramdown core : gettalong/kramdown#644. It got rejected since the change brought little value to the kramdown project.

However, the patch could have relatively more value to Jekyll because:

  • Jekyll Core exclusively uses the :to_html method in its conversion process.
  • Jekyll sites can have a large no. of Markdown documents being converted to HTML.
  • Jekyll sites can have equally large use of the markdownify Liquid filter which under-the-hood uses the same API.

The upstream code as of kramdown-2.1.0 is:

# Check if a method is invoked that begins with +to_+ and if so, try to instantiate a converter
# class (i.e. a class in the Kramdown::Converter module) and use it for converting the document.
#
# For example, +to_html+ would instantiate the Kramdown::Converter::Html class.
def method_missing(id, *attr, &block)
  if id.to_s =~ /^to_(\w+)$/ && (name = Utils.camelize($1)) &&
      try_require('converter', name) && Converter.const_defined?(name)
    output, warnings = Converter.const_get(name).convert(@root, @options)
    @warnings.concat(warnings)
    output
  else
    super
  end
end

@ashmaroli ashmaroli added micro-optimization Matters only if it is a very large site ¯\_(ツ)_/¯ needs-decision 🤔 labels Mar 2, 2020
@ashmaroli ashmaroli requested a review from mattr- March 2, 2020 16:15
@DirtyF
Copy link
Member

DirtyF commented Mar 2, 2020

Memory Profile Summary

--- master https://travis-ci.org/jekyll/jekyll/jobs/657343689
+++ PR     https://travis-ci.org/jekyll/jekyll/jobs/657363607

- Total allocated: 356.24 MB (3209407 objects)
+ Total allocated: 355.73 MB (3200658 objects)
  Total retained:  18.88 MB (107272 objects)

@ashmaroli ashmaroli added this to the 4.1 milestone Mar 16, 2020
@ashmaroli ashmaroli requested a review from DirtyF March 26, 2020 14:03
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.

Awesome!

@ashmaroli
Copy link
Member Author

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 237d08a into jekyll:master Mar 26, 2020
jekyllbot added a commit that referenced this pull request Mar 26, 2020
@ashmaroli ashmaroli deleted the kramdown-doc-to-html branch March 26, 2020 14:11
@jekyll jekyll locked and limited conversation to collaborators Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement frozen-due-to-age micro-optimization Matters only if it is a very large site ¯\_(ツ)_/¯
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants