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

potentially speed up hyphenating style names #3251

Conversation

willheslam
Copy link
Contributor

@willheslam willheslam commented Sep 2, 2020

This works by:

  1. avoiding work if not needed
  2. lower casing individual upper case characters instead of entire strings

I discovered this could be faster when playing around in a repl and measuring the time taken to hyphenate a roughly 66% mix of camelCase names and 33% all lowercase (backgroundColor vs color etc.) (percentage taken from a large-ish project that uses styled-components).

Unscientific perf test

The repl shows it can be around 20 - 80% faster depending on the environment, but is there a way to show this more conclusively?

(in the repl above I got:
255ms vs 143ms
but running in node 12.14.1 I got
162ms vs 124ms, so less dramatic
)

@willheslam willheslam force-pushed the speed-up-hyphenating-style-names branch 3 times, most recently from 28d3b3a to f64bb0b Compare September 2, 2020 15:11
@quantizor
Copy link
Contributor

@willheslam can you merge master? should get the actions running

@willheslam willheslam force-pushed the speed-up-hyphenating-style-names branch from f64bb0b to c471229 Compare September 2, 2020 15:24
by:
1. avoiding work if not needed
2. lower casing individual upper case characters instead of entire strings
@willheslam
Copy link
Contributor Author

@probablyup Done!

@@ -5,8 +5,10 @@
* https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/core/hyphenateStyleName.js
*/

const uppercasePattern = /([A-Z])/g;
const uppercaseCheck = /([A-Z])/;
const uppercasePattern = new RegExp(uppercaseCheck, 'g');
Copy link
Contributor

@quantizor quantizor Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this change would be meaningfully faster

Copy link
Contributor Author

@willheslam willheslam Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For performance reasons it's using Regexp.prototype.test (as opposed to String.prototype.match) and quoting MDN:

As with exec() (or in combination with it), test() called multiple times on the same global regular expression instance will advance past the previous match.

It's a little unclear how it works so I've always avoided it - is each regex storing which strings it's been matched against?
Or is it a per-regex counter for all strings it's matched against?

let str1 = 'fooBarBaz'
let str2 = 'howAboutNow'
let regex = /([A-Z])/g
> regex.test(str1)
true
> regex.test(str1)
true
> regex.test(str1)
false
> regex.test(str1)
true
> regex.test(str2)
true
> regex.test(str1)
false

This seems to imply it's a per-regex counter, so by using a non-global version, the test check isn't internally changing its state.

As further proof of this, changing uppercaseCheck to /([A-Z])/g causes a load of tests to start failing as some names stop becoming hyphenated!

(If there's a better way to solve this problem I'm happy to change it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, fair enough.

@quantizor quantizor merged commit dd6b302 into styled-components:master Sep 2, 2020
@@ -5,8 +5,10 @@
* https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/core/hyphenateStyleName.js
*/

const uppercasePattern = /([A-Z])/g;
const uppercaseCheck = /([A-Z])/;
const uppercasePattern = new RegExp(uppercaseCheck, 'g');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick note here for a minor change; with gzipping it's likely that this will end up being smaller if we just repeat the pattern (with the added g flag) but I realise that this was added in an attempt of deduplication.
I can also confirm that this can have a rather unexpected speed up, due to the reduced internal state, matching state, and lastIndex travelling over from the native irregexp engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my original attempt also duplicated the regex, so if that's preferable I can put together a PR.

I have no idea if JS engines treat /foo/g equivalently to new RegExp(/foo/, 'g') or not - I appreciate that regex performance can be sensitive to slight changes.

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.

None yet

3 participants