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 markdown parsing with Kramdown by reusing the options and parser objects #8013

Merged
merged 5 commits into from Feb 26, 2020

Conversation

ashmaroli
Copy link
Member

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

Summary

Kramdown::Document is designed to set up the necessary options and a parser (based on the provided options) to initiate conversion of a Markdown content string.

But Jekyll uses the same set of options (and therefore the parser as well) to convert all of its Markdown documents. Therefore, having kramdown run the set up tasks for each document is wasteful.

This PR proposes to subclass Kramdown::Document and move the set up tasks from the :initialize method to a singleton_menthod. This approach has the set up tasks run for just the first Markdown document contents converted, cache the resulting objects and subsequently provide these objects to consequent conversions.

@ashmaroli
Copy link
Member Author

Memory Profile Summary

--- master https://travis-ci.org/jekyll/jekyll/jobs/652560205
+++ PR     https://travis-ci.org/jekyll/jekyll/jobs/652608908

- Total allocated: 396.41 MB (3625375 objects)
+ Total allocated: 387.31 MB (3599878 objects)
- Total retained:  19.80 MB (107263 objects)
+ Total retained:  19.80 MB (107275 objects)

@ashmaroli ashmaroli marked this pull request as ready for review February 19, 2020 18:21
@ashmaroli ashmaroli requested a review from a team February 19, 2020 18:21
@DirtyF DirtyF added this to the 4.1 milestone Feb 19, 2020
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.

Well done @ashmaroli 👏

mattr-
mattr- previously requested changes Feb 19, 2020
Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

I can't say I like this, as it means we're now depending upon implementation details in Kramdown that will come back to bite us the next time we need to upgrade Kramdown.

I'd prefer to not see this merged.

@mattr-
Copy link
Member

mattr- commented Feb 26, 2020

I went and looked at the Kramdown code. It's too simple to require further changes. Would be nice to see something like this pushed upstream if they'll agree to it.

@mattr- mattr- changed the title Subclass Kramdown::Document to reduce allocations Optimize markdown parsing with Kramdown by reusing the options and parser objects Feb 26, 2020
@mattr-
Copy link
Member

mattr- commented Feb 26, 2020

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 3d045d2 into jekyll:master Feb 26, 2020
jekyllbot added a commit that referenced this pull request Feb 26, 2020
@ashmaroli
Copy link
Member Author

Awesome!! Thank you very much @mattr- 🎉

@ashmaroli ashmaroli deleted the kramdown-document-subclass branch February 26, 2020 16:39
@jekyll jekyll locked and limited conversation to collaborators Feb 25, 2021
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