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::Document#to_html calls #644

Closed
wants to merge 1 commit into from

Conversation

ashmaroli
Copy link
Contributor

Disclaimer: This optimization is based on an assumption that the major use-case (of this project) is kramdown-to-HTML conversion.

By having :to_html explicitly defined for the Kramdown::Document 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.

@ashmaroli
Copy link
Contributor Author

@gettalong
Copy link
Owner

Thanks for this pull request - do you have some real world benchmark numbers where this change actually makes a difference?

@ashmaroli
Copy link
Contributor Author

real world benchmark numbers where this change actually makes a difference

Since this is a micro-optimization, I doubt there would be any perceptible improvement in performance for real world scenarios.
That said, if you'd like to see some memory profiling numbers, the following are from profiling the documentation site of Jekyll project (which involves 696 calls of Kramdown::Document.new(content).to_html):

--- before
+++ after

- Total allocated: 356.30 MB (3209408 objects)
+ Total allocated: 355.79 MB (3200658 objects)
- Total retained:  18.88 MB (107273 objects)
+ Total retained:  18.88 MB (107272 objects)

@gettalong
Copy link
Owner

Thanks for your comment!

In this case I don't think that this micro-optimization is useful because it doesn't bring us much and unnecessarily duplicates code.

@gettalong gettalong closed this Mar 2, 2020
@ashmaroli
Copy link
Contributor Author

Thank you for looking into the proposal anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants