Skip to content

Conversation

@ThePuzzlemaker
Copy link
Contributor

@ThePuzzlemaker ThePuzzlemaker commented Aug 3, 2021

I'd rather not copy out the entire description of the previous iteration, so I'll just link it here: #1494

This iteration has much better performance than the previous iteration:

Command Mean [s] Min [s] Max [s] Relative
Syntect v2 3.512 ± 0.020 3.489 3.554 2.62 ± 0.03
Syntect v1 12.404 ± 0.156 12.141 12.597 9.24 ± 0.14
HLJS 1.342 ± 0.011 1.325 1.355 1.00

(measured on my machine using hyperfine, building the Rust Book; "Syntect v1" = #1494, "Syntect v2" = this fork/PR, "HLJS" = upstream mdBook)

There's still some performance improvements to be made, and a lot of it seems to be from hbs_renderer::add_playground_pre. This function searches through the entirety of the HTML content of a page and searches for Rust code blocks which need a "preamble" (containing an fn main() {} that wraps the original code). However, this is done after code is syntax highlighted, and the resulting code is quite verbose thanks to how syntect highlights code (this is more due to how tmLanguage/sublime-syntax definitions work, than to the implementation of syntect, however). This means that searching through a bunch of code blocks is quite slow in practice.

As I noted here, this step could be moved to before syntax highlighting, which would not only reduce the number of edge cases and weirdness due to odd syntax highlighting, but would also probably be faster. I'll probably do this at some point, and see if it actually helps with performance. Based on my profiling, though, it seems that this function is the major performance issue.

TODO

As this is such a major change to the Handlebars backend, here is a list of things that should be done before this is minimally useful in production:

  • Improve performance, just generally
  • More testing and bugfixes
  • Make sure CSS files are free of any possible bugs from syntect's sublime theme to CSS generator
  • Add playground preambles before syntax highlighting, and test if that actually improves performance
  • Update documentation accordingly

@ThePuzzlemaker
Copy link
Contributor Author

As for why I didn't implement this on top of my old changes: I didn't feel like it.
I felt that it would be better to start from a clean upstream rather than trying to rebase a bunch of stuff on top of an old fork and then making my changes after then.

@ThePuzzlemaker
Copy link
Contributor Author

Woops, I just realized I never actually checked the output in the browser and there are a few problems. I'll fix those 😅

@ThePuzzlemaker
Copy link
Contributor Author

ThePuzzlemaker commented Aug 3, 2021

Hm, so looks like I'm going to have to rethink how I do line hiding.
Basically syntect doesn't actually work line-by-line; due to how tmLanguage/sublime-syntax definitions parse things, it means that the entire code block is one giant <span>.

@ThePuzzlemaker
Copy link
Contributor Author

Closing in favor of #1652.

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.

1 participant