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

Hash for css class names is slow #536

Closed
blackswanny opened this issue Jan 13, 2018 · 7 comments · Fixed by #544
Closed

Hash for css class names is slow #536

blackswanny opened this issue Jan 13, 2018 · 7 comments · Fixed by #544

Comments

@blackswanny
Copy link

Good day. We use glamor, however when amount of css classes is huge, it starts to slow down performance, because it's murmur hash function is very strong, but very slow
Before switching to emotion we ask , if you use the same one. Seems like yes.
https://github.com/emotion-js/emotion/blob/master/packages/emotion-utils/src/hash.js

@mxstbr
Copy link
Contributor

mxstbr commented Jan 13, 2018

/cc @threepointone

@threepointone
Copy link
Contributor

glamor's moving to a faster hash function soon (threepointone/glamor#294)

also consider hoisting styles/objects to a higher scope than render calls/tight loops

@blackswanny
Copy link
Author

@threepointone which one you chose? So we can try to apply it and send feedback ahead.
The one I found working for our big project is below, it's very simple
https://github.com/darkskyapp/string-hash/blob/master/index.js

@tkh44
Copy link
Member

tkh44 commented Jan 14, 2018

I tried all the hashing algorithms and methods. They all have to loop over the string at least once. The underlying math is not that much faster. It all centers around how many times the string is going to be iterated over.

@threepointone sha-1 is not going to be faster. I'm fairly certain of this. All of the advantages seen in the implementation of threepointone/glamor#294 are around adding another layer of caching.

Hashing is slow and I've wondered about this for a while. I'm not sure of the right solution. I wish there was a native hash function we could use.

https://github.com/emotion-js/emotion/pull/336/files#diff-b3a9ec97edd00bfb96d42e8e41cf409e

I experimented in that PR and could not find any reason to switch. Only advantage is that that implementation is a bit smaller.

@tkh44
Copy link
Member

tkh44 commented Jan 14, 2018

We have a bit more complex caching setup so it is not as big of a deal. The biggest advantage of emotion is that you can break up your styles into component parts and then combine them into very large styles and get some more performance boosts.

@blackswanny
Copy link
Author

@tkh44 We have measured that primitive hash and compared with original glamor one on our project. It's about 16K classes. So it increased up to 4 times. We use glamor as a part of React. We have Base component, which works with glamor directly, so all other components inherit from this one. We found that every usage of this Base component cost us a lot, because hash of glamor is called all the time. We dont want to and cant change approach, if it's an issue also. Optimizations, which may cause worse code readability may not be accepted.
cc @threepointone

@tkh44
Copy link
Member

tkh44 commented Jan 15, 2018

@blackswanny You are correct. I didn't project the size/cost relationship that far out. If you put in a PR to change the hash and it all looks good I feel no hesitation to merge it. The diff might be pretty large depending on how much this affects the snapshots but don't let that stop you. Need to keep in mind how this affects the hashes. This could be a big pain for some devs to update all their snapshots. I think it will be fine, but something that we have to stay aware of.

I would also mention that because of the nature of a WeakMap and references created in render, etc string based styles can be significantly faster due to cache hits. If you are heavily using dynamic styles you could also see a boost here. Like @threepointone said, hoisting can bring parity in some cases for objects.

https://github.com/emotion-js/emotion/tree/master/packages/babel-plugin-emotion#hoist

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 a pull request may close this issue.

4 participants