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.

Sign up for GitHub

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

Jump to bottom

Ignore CSS Custom properties in kebabifyStyleName #340

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ignore CSS Custom properties in kebabifyStyleName #340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9e4b78
Ignore CSS Custom properties in kebabifyStyleName
dmiller9911 Aug 14, 2018
1d23ca0
Use manual lookup for --
dmiller9911 Aug 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ const UPPERCASE_RE = /([A-Z])/g;
const UPPERCASE_RE_TO_KEBAB = (match /* : string */) /* : string */ => `-${match.toLowerCase()}`;

export const kebabifyStyleName = (string /* : string */) /* : string */ => {
if (string.substring(0, 2) === '--') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i tried using string.startsWith('--') here, but I did not see if being transpiled at build time. I messed with the babel config a little, and had no luck.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's because startsWith needs to be polyfilled since it is a prototype method and Babel doesn't have type information available when compiling.

This function is in a very hot path, so we want to make sure we do this as efficiently as possible. Can you update to this?

if (string[0] === '-' && string[1] === '-') {
  return string;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lencioni Awesome that makes sense. Out of curiosity I made a jsperf for this check, and my original came out much faster in chrome than the suggested. However in FF and Safari your suggested solution was faster. I do not have the ability to run it in Edge/IE. What browser do you mainly suggest targeting in terms of performance? I assume chrome has the larger market share for most things, but that may not always be the case. I definitely don't mind changing this, and I more asking for future changes and personal growth.

JS Perf: https://jsperf.com/is-css-custom-prop/1

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think your jsperf is maybe benefitting from an optimization that is happening because it is always running on the same string. I made a different jsfiddle (I tried to edit yours, but jsfiddle seems to be broken for that at the moment) that tests on a random string.

https://jsperf.com/is-css-custom-prop-2/1

While working on this, I noticed that substring is faster than startsWith, but slower than manual lookup and slice when comparing with an inline string (=== '--') but is as fast as slice and faster than manual lookup when comparing with a string variable (=== twoDashes).

This kind of optimization can be a bit of a moving target as browsers make improvements. I originally did this in March 2017, so it is totally possible that things are different now in Chrome. f907752

Also, I don't remember exactly how I tested the perf of this at the time, but it is likely that I tested using Aphrodite itself instead of in a microbenchmark on jsperf, which would give a more realistic analysis.

I did, however, find this jsperf I made for testing a different optimization in Aphrodite, which seems to no longer hold up, so it might be worth revisiting that as well. https://jsperf.com/slice-vs-bracket-string-check-with-imporant Although that might have the same issue of reusing the same string.

In the end, it might not really make much difference and the best way to know for sure is to test real world performance directly here. If you don't like the manual approach, I'd suggest .slice() instead of .substring() because it has fewer quirks in general.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It occurs to me that it would be really useful for aphrodite to have some benchmarks running in puppeteer or something. Does that sound like something you'd like to contribute, in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be good. What are your thoughts about this benchmark:

Hosted: http://necolas.github.io/react-native-web/benchmarks/
Implementation: https://github.com/necolas/react-native-web/tree/master/packages/benchmarks/src/implementations/aphrodite

I am definitely good with the change you proposed. More just curious on the perf stuff. Will make that change and look into adding benchmarks to this repo too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent! Let's go with the manual lookup approach, to keep everything consistent for now. Then, once we get good benchmarking in, we can iterate on performance micro-optimizations all at once.

That benchmark looks like a pretty good starting point to me, but I have some thoughts.

It looks like it runs the benchmark code a fixed number of times (50), so I'm not sure about the statistical significance. It seems like it would be better for our case if it ran the benchmark until it achieved significance (caveat: I know almost nothing about stats). I think Benchmark.js is designed for this type of analysis, but I haven't used it before. Anecdotally, I seem to get pretty different numbers every time I run the benchmark.

I like that there are different types of test cases (wide tree, shallow tree) and I like that it tries to approximate time spent in scripting vs layout (it looks like they force layout here to get that timing). However, for aphrodite at least, there are a couple of things that this misses.

aphrodite has a step that will inject the styles onto the page the first time they are used, immediately after rendering (using the asap package). In profiling the benchmarking tool you linked to, it looks like this only happens the first time the component is rendered, which is when you select "aphrodite" from the dropdown, which is before the test starts. So this pretty important step isn't captured by this benchmark very well.

This might not be as big of a problem if there is a CLI interface, since it would be captured by the benchmark once. But, even if it is included once, it doesn't not isolating it specifically, which I think is important for our benchmark.

It also looks like the forcing of the layout would happen before the styles are inserted, so that would need to be remedied. I bet this could be fixed by using asap inside componentDidUpdate to defer the forcing of the layout until after the styles are injected. This probably only matters for when we are measuring the style injection itself.

It looks like the styles aren't contained, so when the browser does layout, the layout root is the document, so more nodes are counted than what is inside the benchmark box. There are not many nodes outside the benchmark box, so this is probably a very minimal overhead and might not matter, but it is something I noticed. This could be easily addressed in this tool by using CSS containment (for browsers that support it).

performance tests 2018-08-23 03-21-44

It isn't clear to me how useful it is for the benchmark to be using React. On one hand, it is nice to have a real-worldish scenario, but on the other hand, it might make isolating different aspects a little more difficult and the results might be muddied by aspects of React perf. I think this is probably mostly useful for the layout perf calculations, since it will actually be rendering DOM.

I think that tool probably works well for its purpose, which is to "provide an early-warning signal for performance regressions", but we are looking for something that can help us squeeze out every little millisecond of performance, so I think we want something a little more tuned.

I think I would probably go with a benchmark that tests:

  • new stylesheet creation (i.e. start up time, without mounting or rendering anything)
  • new stylesheet injection (i.e. initial mount of new component)
  • stylesheet re-use (i.e. component re-rendering, or re-mounting a previously mounted component)

I'd probably look into using Benchmark.js or a similar tool that will run the tests until they achieve statistical significance. It would be nice if I could run it from the command line (maybe using puppeteer) and also in a browser (so I can test it in various browsers if I like).

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, great write up. I think that sounds great. I made the change will start looking into the benchmarking in the next few days. Thanks for the insights.

return string;
}
const result = string.replace(UPPERCASE_RE, UPPERCASE_RE_TO_KEBAB);
if (result[0] === 'm' && result[1] === 's' && result[2] === '-') {
return `-${result}`;
Expand Down
3 changes: 3 additions & 0 deletions tests/util_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,8 @@ describe('Utils', () => {
it('forces -ms-', () => {
assert.equal(kebabifyStyleName('msFooBarBaz'), '-ms-foo-bar-baz');
});
it('does not kebabify CSS Custom properties', () => {
assert.equal(kebabifyStyleName('--foo-Bar_Baz'), '--foo-Bar_Baz');
});
});
});