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

Exported vars from Tokenizer.js causing downstream performance issues #278

Closed
quintstoffers opened this issue Oct 26, 2021 · 2 comments
Closed

Comments

@quintstoffers
Copy link

I'm running into a performance issue caused by a combination of https://github.com/benjamn/reify and stylis in a Meteor application.

Tokenizer.js exports variables that are frequently updated during runtime. This triggers reify to do additional work, which causes massive performance hits rendering components through emotion (which uses stylis). Rendering components takes over 20 times longer because of the reify overhead updating the exported vars.

It seems like these exports aren't used anywhere:
https://github.com/thysultan/stylis.js/blob/e6843c373ebcbbfade25ebcc23f540ed8508da0a/src/Tokenizer.js#L3-L8

However, they are exported from the package:
https://github.com/thysultan/stylis.js/blob/e6843c373ebcbbfade25ebcc23f540ed8508da0a/index.js#L5

Removing them would technically be a breaking change, although I can't imagine anyone making use of them. Was exporting these variables intentional?

@Andarist
Copy link
Collaborator

I'm actually using position in Emotion 😅 :
https://github.com/emotion-js/emotion/blob/4be3391914878fd41279701bc23332b75903ec43/packages/cache/src/stylis-plugins.js#L11

While I sympathize with your issue, it really seems to be more of a reify issue than Stylis, right? Is there any potential of improving this there? Maybe worth reporting the performance cliff there? What kind of work are we talking about? Why do non-consumed live bindings add so much overhead there?

@quintstoffers
Copy link
Author

I'm actually using position in Emotion 😅 :

Bummer 😄

it really seems to be more of a reify issue than Stylis

I agree it's mostly a reify "issue", but I don't immediately see how their process could be improved. It needs to mimic some behaviour that comes native with the import/export syntax and since the Tokenizer code modifies the exported variables it needs to do it's magic. I don't think it knows whether an import is actually used or not, and I'm not sure if it could.

As for these variables being exported and used, I haven't really explored this pattern in the past. To me it seems a little unorthodox and I feel like these variables should be exposed through some sort of API instead of just being exported. Some framework for a plugin to work in, basically. However, that doesn't invalidate the approach and I don't have nearly enough experience with the current use cases or history of this package to suggest an alternative.

Either way, this issue was mostly me trying to figure out what's going on here and to figure out if these exports are intentional. It sure looks like they were so I guess we'll be looking for other short-term options for our performance issues. Removing the potentially unused exports was just the easiest way 😉

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

No branches or pull requests

3 participants