rtfeldman / elm-css

Typed CSS in Elm.
https://package.elm-lang.org/packages/rtfeldman/elm-css/latest
BSD 3-Clause "New" or "Revised" License
1.23k stars 196 forks source link

New hash scheme #555

Closed robinheghan closed 2 years ago

robinheghan commented 2 years ago

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

Previously the flow worked something like this:

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

robinheghan commented 2 years ago

I’ll fix it :)

robinheghan commented 2 years ago

All tests pass, ready to merge :)