Core: add jQuery version check utility method #299
Conversation
|
|
||
| // Returns 0 if v1 == v2, -1 if v1 < v2, 1 if v1 > v2 | ||
| function compareVersions( v1, v2 ) { | ||
| var rVersionParts = /^(\d+)\.(\d+)\.(\d+)/, |
There was a problem hiding this comment.
I'm relatively new to game of "optimise for delivery size", but I'll try to jump in here based on what I see us do elsewhere.
I wonder if it'd make sense here to omit the variable in favour of inlining the regex literal in the two places we need it. It would probably be the same or maybe slightly bigger after minification/obfuscation, but after gzip I'd expect it to be slightly smaller that way. Execution speed would presumably be the same or better same. Historically I'd think "parse twice" is slower than "parse once, store, reference twice". But given how regex literals work in JS, I think it might even run slightly faster without the variables.
Anyhow, super low priority :) I'm just testing out what our reduction thoughts are. I imagine it's generally more important in core than in migrate. (Although Wikipedia and WordPress do both run Migrate in production for prolonged periods of time, given the need to support user-supplied scripts.)
| v2p = rVersionParts.exec( v2 ) || [ ]; | ||
|
|
||
| for ( var i = 1; i <= 3; i++ ) { | ||
| if ( +v1p[ i ] > +v2p[ i ] ) { |
There was a problem hiding this comment.
This isn't a hot code path, but given the choice to add explicit number casting here, I figured I'd run some benches anyway. I was expecting to conclude how much faster it is. Especially in Chrome given the history of optimising for asmjs and more generally with the high "type" confidence.
To my surprise, it turned the other way around. Implicit number casting consistently performs faster than explicit number casting. Up to 40X even.
I do apologize for the below table's size. It's was more an exercise in curiosity of comparing the absolute performance of the browsers and devices involved. In terms of relative change, it's pretty similar across the board (small difference in Firefox, medium difference in Edge, huge leap in Safari).
| Explicit | Implicit | Change | |
|---|---|---|---|
| Chrome 65 | 14M ops/s | 110M ops/s | x8 |
| Chrome 67 canary | 14M ops/s | 109M ops/s | x7 |
| Firefox 58.0.2 | 3.9M ops/s | 4.5 ops/s | +15% |
| FirefoxNightly 60.0a1 nightly | 3.7M ops/s | 4.1M ops/s | +10% |
| FirefoxDeveloper 60.0b6 aurora | 2.6M ops/s | 3.5M ops/s | +34% |
| Safari 11.0.2 | 5M ops/s | 187M ops/s | x37 |
| Safari 11.2 (TP Release 52) | 4M ops/s | 157M ops/s | x40 |
Above locally on a MacBook, without throttling. Below from BrowserStack.
| Explicit | Implicit | |
|---|---|---|
| IE 11 (Windows 10) | 3.6M ops/s | 11.1M ops/s |
| Edge 40 (EdgeHTML 15; Windows 10) | 3.7M ops/s | 7.0M ops/s |
| Android 4.4 (Chrome; Galaxy S4) | 1.9M op/s | 4.1M ops/s |
| Android 7.0 (Chrome; Galaxy S8) | 7M ops/s | 14M ops/s |
There was a problem hiding this comment.
The Safari and Chrome numbers look so high that I think it just optimized away the empty if statements completely for those cases. Not sure why it wouldn't do so for the explicit ones too.
There was a problem hiding this comment.
@dmethvin Good point. However, am I correct in understanding you added the explicit casting as means to improve performance? Even without the 40X difference, or if it were 40X the other way around, it seems fast enough both ways (millions per second), to not be worth optimising with an explicit cast. Or can behaviour differ between the two?
There was a problem hiding this comment.
Oh! For the version check you have to compare the major/minor/patch as numbers or they come out wrong. That is, we want "3.2.1" to be less than "3.11.0" but if you do a string comparison you get "2" > "11" which is wrong. I realized there was no test case for that so I just added one.
2 participants
This is needed so that Migrate can avoid false warnings for the internal uses of public methods that are being removed during updates to jQuery 3.x.x. Several of the upcoming PRs need to use it. I've only exposed
jQueryVersionSincefor the actual code but I do testcompareVersionsin the unit tests just to be sure it's sane. Not sure if there's a better way to do this, suggestions welcome.