Optimize escaping of IDs

[aickin]

I did some preliminary profiling of React 16 SSR, and I found something very similar to what I found in #6862, that escaping IDs was taking an inordinate amount of time. I wrote a similar solution that shortcuts for strings that don't have characters to escape and loops over matches. With this new implementation, I was able to make my (admittedly rather synthetic) SSR benchmark run about 20% faster.

I tried to run the react benchmark in scripts/bench to see if it agrees with my benchmark, but it gives me errors that the chrome-launcher module doesn't exist. I think this is because lighthouse's code has been reorganized, and fixing the benchmark seemed a little out of scope for this PR. If anyone knows how to get the benchmark running, I'd appreciate the info!

[sophiebits]

Thanks! I’ll take a look later. Does #11749 solve your benchmarking problems?

[aickin]

Does #11749 solve your benchmarking problems?

I updated from master today, so I don't think so, but I'll double check.

[aickin]

Two things:

1. I wrote this a month or two ago and only came back to it today. On second look, I have no idea what I was thinking in my rewrite of the function; it was wildly incorrect. I added some test cases and fixed the implementation in 105e925.
2. Upon further reflection, I realized this may be a universally relevant optimization. I realized that this code only runs for keys, and in my test benchmark basically every element has a key. While I think it shouldn't hurt code that doesn't use keys, it'll only really be useful for ones that make a lot of use of keys. Given that my implementation is a bit more complicated than the previous implementation, it may not be worth the perf gain. Please feel free to tell me what you think on this question!

[syranide]

It seems you're maybe not merging this, but would it not make sense to instead just do a pre-check and if there's a match then invoke the function as-is, otherwise return the passed string. The expansion of the replace code is quite a lot more complex and difficult to read, and I doubt it has any meaningful impact (whereas having the pre-check does have an impact).

I'm even surprised it would be faster than just letting the native replace-code do its thing, this is certainly a performance flaw in the browser JS engine and there's every reason to believe the native function will be faster in the future.

[aickin]

would it not make sense to instead just do a pre-check and if there's a match then invoke the function as-is, otherwise return the passed string.

Great idea, and I'd be happy to do that!

I'm even surprised it would be faster than just letting the native replace-code do its thing, this is certainly a performance flaw in the browser JS engine and there's everyone reason to believe the native function will be faster in the future.

It's been this way for a while, at least since #6862, which was about 18 months ago. I also think there's a chance that it won't get better soon, as regex logic is probably already written in C and has to handle an enormous number of possible cases, which can make optimizations like this hard and/or low priority. But that's totally a guess!

[gaearon]

@aickin What's your latest thinking on this? Is it worth pursuing?

[aickin]

Eh, I don't really know. It definitely helps my synthetic benchmark, but I've grown less and less confident that that's important; it's really synthetic. I think we should probably close this until there's a better, more real-world SSR test suite available.

[gaearon]

until there's a better, more real-world SSR test suite available.

FWIW this would be really great to have for testing other things too. Maybe it's worth focusing some effort on creating one? cc @rauchg Would somebody from Next be interested in helping?