-
Notifications
You must be signed in to change notification settings - Fork 188
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
Improve importantify()
performance
#201
Conversation
On my phone (Pixel), function hoisted is the winner with function and string being slowest. |
I think we can also simplify the regex here. If we change the code to this: const IMPORTANT_RE = /(?: !important)?;$/;
// Given a single style rule string like "a: b;", adds !important to generate
// "a: b !important;".
export const importantify = (string /* : string */) /* : string */ =>
string.replace(IMPORTANT_RE, " !important;"); it drops the runtime of |
FYI, I'm trying out all of my optimization PRs at the same time, and together they seem to speed up |
FYI, I also dug into inline-style-prefixer a little, and opened some PRs over there that should really help out as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! This looks like a big improvement! I guess our importantify
wasn't used enough to require something so complex. Thanks!
One small style nit, and we can get this landed :)
src/util.js
Outdated
export const importantify = (string /* : string */) /* : string */ => ( | ||
string.slice(-11) === ' !important' | ||
? string | ||
: `${string} !important` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we use 4 space indents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
99fbfec
to
bd0a575
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks great! I'd love to see a comment explaining what's going on in that check, though.
src/util.js
Outdated
// Given a single style value string like the "b" from "a: b;", adds !important | ||
// to generate "b !important". | ||
export const importantify = (string /* : string */) /* : string */ => ( | ||
string[string.length - 10] === '!' && string.slice(-11) === ' !important' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hehe this is great! Could you maybe add a comment explaining why this check looks so funky? Also maybe wrap these checks in parens because I can never remember the precedence of ?:
? :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
src/generate.js
Outdated
@@ -286,8 +286,7 @@ export const generateCSSRuleset = ( | |||
|
|||
const rules = prefixedRules.map(([key, value]) => { | |||
const stringValue = stringifyValue(key, value); | |||
const ret = `${kebabifyStyleName(key)}:${stringValue};`; | |||
return useImportant === false ? ret : importantify(ret); | |||
return `${kebabifyStyleName(key)}:${useImportant === false ? stringValue : importantify(stringValue)};`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is a bit long. :( Maybe split it up into two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
I was rolling through the code here looking for some opportunities to speed it up and I noticed that there is a capture group in this regex that isn't being used. By changing `(` to `(?:` we can avoid capturing this group while still being able to use the parens for grouping.
I was curious about the performance characteristics of various options here, and using a string in this way seems to have the edge, at least in Firefox (35% faster) and Safari (12% faster)--Chrome performance seems pretty similar for all possibilities. I also find this version more readable, so it seems like we may as well go with this option. https://jsperf.com/replace-string-vs-function/
I don't think we actually need a capture group at all here and can accomplish the same thing by just anchoring on the end of the string. This reduces the runtime of this function in my benchmark from ~105ms to ~35ms.
Since we only call this function from one place, and we already have the CSS property name and value split out, we can further optimize this hot path by only running importantify on the CSS property value. In my benchmark, this change speeds up this map by ~22% (~128ms to ~100ms).
Now that this has been sufficiently simplified, we can easily remove the regex entirely.
In our microbenchmark, adding this optimization performs a good amount better. Since it is a simple optimization to add, it seems like we may as well do it. https://jsperf.com/slice-vs-bracket-string-check-with-imporant I considered using bracket notation for the whole check, but I decided that was too verbose. So instead, I decided to just check for one character and follow up with a more thorough but concise check afterward. This optimization favors the case where the style does not already have `!important` on it, which is by and large likely to be the general case.
This cleans up the code a bit, and is also faster! https://jsperf.com/if-vs-extra-function/1
I was rolling through the code here looking for some opportunities to
speed it up and I noticed that there is a capture group in this regex
that isn't being used. By changing
(
to(?:
we can avoid capturingthis group while still being able to use the parens for grouping.
I was also curious about the performance characteristics of various
replace
arguments, and using a string in this way seems to have the edge, at least in
Firefox (35% faster) and Safari (12% faster)--Chrome performance seems
pretty similar for all possibilities. I also find this version more
readable, so it seems like we may as well go with this option.
https://jsperf.com/replace-string-vs-function/
I don't think we actually need a capture group at all here and can
accomplish the same thing by just anchoring on the end of the string.
This reduces the runtime of this function in my benchmark from ~105ms to
~35ms.