speed improvement: cache regexp

[adriengibrat]

In Mithril code source regexp are never cached, they alway written as literals inside function body.
This could be optimised, in particular inside parseTagAttrs, which is called each time m is called.
See https://github.com/lhorie/mithril.js/blob/69f90deb64cc90401a2f172c274e5c78921c80d9/mithril.js#L87

Would you accept a PR ?

[adriengibrat]

BTW, the regexp in routeUnobtrusive at https://github.com/lhorie/mithril.js/blob/69f90deb64cc90401a2f172c274e5c78921c80d9/mithril.js#L1711
seems to target parent link tag, but will match any tag with the letter a (table, area, acronym, etc.)

smells like a little bug ...

[leeoniya]

Pretty sure all modern javascript JITs already cache immutable regexes (all that are defined using literal notation). Pulling regexes out of the functions for which they are made is very likely unnecessary and definitely reduces readability.

Have you measured the perf difference? Maybe it benefits older browsers with shittier JITs.

[tinchoz49]

I execute this code in chrome:

function isFooBar(str) {
  return str.match(/Foo|Bar/)
}
console.time('1');isFooBar("Foo");console.timeEnd('1');
console.time('2');isFooBar("Bar");console.timeEnd('2');

and the result was:

VM173:5 1: 0.141ms
VM173:6 2: 0.017ms

So i think the javascript JITs already cache immutable regexes, at least the V8.

[adriengibrat]

@tinchoz49 you just pinpointed a match can takes more time than a rejection

The test should be more like http://jsperf.com/regex-caching-lp/2 (see less/less.js#2501)

Speed improvement may be small but given the number of times m and other functions can be called, they are a few place where this little optimization could be done.

And also, naming regexp is not easy, but can improve readability.

EDIT: you slighly updated your test. You're right, it seems V8 caches regexp, and may be results:

function isFooBar(str) {
  return str.match(/Foo|Bar/)
}
console.time('1');isFooBar("Foo");console.timeEnd('1');
console.time('2');isFooBar("Bar");console.timeEnd('2');
console.time('3');isFooBar("Foo");console.timeEnd('3');
console.time('4');isFooBar("Bar");console.timeEnd('4');

VM99:5 1: 0.386ms
VM99:6 2: 0.061ms
VM99:7 3: 0.047ms
VM99:8 4: 0.047ms

On Chrome Version 49.0.2623.75 (small atom proc ;)

Can we discard optimization because V8 does it, what about other browsers (IE, Safari) ?

[leeoniya]

I tried this optimization about an hour ago [1] in domvm and reverted it [2] since there was no measurable difference in real-world perf [3]. In both cases it's very far from being any sort of bottleneck in the lib. Feel free to flip between the two commits in the repo. I assume the same holds true for Mithril.

[1] domvm/domvm@db874a0
[2] domvm/domvm@de0dd1f
[3] http://leeoniya.github.io/domvm/test/bench/dbmonster/

[adriengibrat]

@leeoniya i'm sure you're right: most javascript engines caches litteral regexes

V8 may not need this kind of optimization, unless this comment is true, any way let's agrgeed this is an optimization for "shittier" JS Engines

I just can't find any serious (multiple browsers, including IE, Edge, Safari, Android, etc) tests about this...

[lhorie]

Just tried and yes the difference is smaller than per-run variances, so pretty much negligible in Chrome.

The routeUnobtrusive test does look weird though

[adriengibrat]

I've made a separate PR #1009 about routeUnobtrusive.

Again, I know it's not an issue in Chrome, we all aggreed on this.

The point is, is it on other browsers? Do we care about it?

[adriengibrat]

I've run some more tests with various browsers (mobile, IE11, etc) via crossbrowsertesting.com on http://jsperf.com/regex-caching-lp/2

They are just few versions of chrome where the difference is sensible (I guess, this may be explained by less/less.js#2501 (comment))

A lot of mobile and "old" (i.e. not evergreen) browsers do optimize this, so i'm closing the issue.

[dead-claudia]

@adriengibrat To be honest, if you really wanted to speed up the selector parsing, you shouldn't even be using regular expressions in the first place. The only reason why I haven't implemented that in core is because I'm concerned about size (it could literally add upwards of a kilobyte alone to the library), and with current tooling, it's not practical to split that out as two separate, complementary implementations. (I'm hoping I can do alternate implementations with @lhorie's rewrite, though.)