Ignore CSS Custom properties in kebabifyStyleName

[dmiller9911]

Allow setting CSS Custom property values in various casing with StyleSheet.create/css by ignoring those keys in kebabifyStyleName utility

[dmiller9911]

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.

[lencioni]

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;
}

[dmiller9911]

@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

[lencioni]

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.

[lencioni]

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?

[dmiller9911]

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.

[lencioni]

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).

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?

[dmiller9911]

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.