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

New hash scheme #555

Merged
merged 11 commits into from
Jan 3, 2022
Merged

New hash scheme #555

merged 11 commits into from
Jan 3, 2022

Conversation

robinheghan
Copy link
Contributor

This PR makes a big change to how classNames are generated.

Previously the flow worked something like this:

  • When user creates a node (div, span, whatever)
    • Compile CSS styles to CSS without class selector
    • Hash compiled css to get class name
  • When converting to "unstyled" html
    • Compile CSS styles to CSS with class selector retrieved from hash

There are some potential performance improvements to this flow:

  1. CSS styles are potentially compiled twice (once without and once with a class name)
  2. All styled nodes are hashed, meaning if you have a list of 100 equally styled elements, we hash their styles 100 times

This PR tries to improve performance by:

  • When a user creates a node, we compile the CSS styles to CSS with a template selector
  • We use this compiled CSS as the key in a dictionary of Styles -> Hash. The style is only hashed if it isn't already in the dictionary.
  • Instead of re-compiling the CSS, we perform a String.replace on the compiled css, replacing the template selector with the hashed class name.
  • To reduce the overhead of dictionary lookups and hashing, the pretty printer uses a much more compact representation than before. I guess one could argue that it doesn't really pretty print anymore.
  • Also reduce the number of allocations required when compiling CSS.

Somewhat unrelated, this PR also replaces a bunch of String.join calls with direct string concatination (++) where possible, as String.join shows up as a hot spot when profiling.

Depending on the browser, this change represents a 18-30% improvement compared to the current master branch.

@robinheghan
Copy link
Contributor Author

Some questions:

  1. There are more spaces that could be removed from the pretty printed output. Less output means faster dict lookup and faster hashing. I kept some of them in, just so that the tests wouldn't be as cryptic. Do you think I should remove the few spaces that remain?
  2. Do you have a suggestion for a better classnameStandin value? It should preferably be short, but it also has to be unique so that we don't replace something important.

@brian-carroll
Copy link

Cool!
For the stand-in class name, how about a single unprintable control character like the bell character "\a"? It's as short as it gets and it's probably invalid CSS, or at least unlikely to be used by a sane person.

@robinheghan
Copy link
Contributor Author

New commits produce even less whitespace in generated css for faster hashing and dict lookups.
Also took Brian's suggestion to use '\a' as the standin classname.

@rtfeldman
Copy link
Owner

@robinheghan Looks like there are some merge conflicts since I merged your other PR. 😅

@robinheghan
Copy link
Contributor Author

I’ll fix it :)

@robinheghan
Copy link
Contributor Author

All tests pass, ready to merge :)

@rtfeldman rtfeldman merged commit 2e2b61c into rtfeldman:master Jan 3, 2022
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.

3 participants