Performance Improvements for string().guid()#1211
Performance Improvements for string().guid()#1211Marsup merged 3 commits intohapijs:masterfrom DavidTPate:guid-perf
Conversation
…as well as when versions are specified.
|
According to my tests, it seems to be an improvement on node 6, but it's very much the opposite for node 8, not sure what to do here. I'd tend to favor the future version. What do you get on your machine ? |
|
That's very interesting as I noticed improvements on Node 8 as well. Node 6.10.3 (Fedora 24 - 64 bit) Node 6.10.3 (Fedora 24 - 64 bit) - New Code Node 8.0.0 (Fedora 24 - 64 bit) Node 8.0.0 (Fedora 24 - 64 bit) - New Code |
|
Forgot to follow up on this as well:
Definitely agree. |
|
Confirmed improvement on 8.1 ¯\_(ツ)_/¯ I'll review soon. |
Marsup
left a comment
There was a problem hiding this comment.
Not sure if it requires changes as most of my comments are minor. Let me know if you think any of those is relevant.
lib/types/string/index.js
Outdated
| } | ||
|
|
||
| const regex = /^([\[{\(]?)([0-9A-F]{8})([:-]?)([0-9A-F]{4})([:-]?)([0-9A-F]{4})([:-]?)([0-9A-F]{4})([:-]?)([0-9A-F]{12})([\]}\)]?)$/i; | ||
| const guidRegex = new RegExp(`^([\\[{\\(]?)[0-9A-F]{8}([:-]?)[0-9A-F]{4}\\2?[${checkVersion ? versionNumbers : '0-9A-F'}][0-9A-F]{3}\\2?[${checkVersion ? '89AB' : '0-9A-F'}][0-9A-F]{3}\\2?[0-9A-F]{12}([\\]}\\)]?)$`, 'i'); |
There was a problem hiding this comment.
I long for named capturing groups...
There was a problem hiding this comment.
You and me both, I can't tell you how many times I counted and recounted these things as I was making incremental changes.
lib/types/string/index.js
Outdated
| versions[versionNumber] = ''; | ||
| } | ||
|
|
||
| versionNumbers = versionNumbers.join(''); |
There was a problem hiding this comment.
Isn't it a deopt to change a var type ?
There was a problem hiding this comment.
Yeah, I think I was just having a bout of temporary idiocy. Instead of versionNumbers.push(versionNumber) and versionNumbers = versionNumbers.join('') I could just replace line 361 with versionNumbers += versionNumber so that I am left with my concatenated string.
The purpose of these parts is solely to be used for building out the RegExp.
lib/types/string/index.js
Outdated
| versions.push(version); | ||
| const versionNumber = uuids[version]; | ||
| Hoek.assert(versionNumber, 'version at position ' + i + ' must be one of ' + Object.keys(uuids).join(', ')); | ||
| Hoek.assert(!(versionNumber in versions), 'version at position ' + i + ' must not be a duplicate.'); |
There was a problem hiding this comment.
hasOwnProperty is usually faster, but that'd be marginal. Think a Set would do better ?
There was a problem hiding this comment.
I'd be interested to see. If it's marginal either way I would prefer a Set as I think it will be easier to comprehend the entire block of code.
lib/types/string/index.js
Outdated
| const versions = []; | ||
| let versionNumbers = []; | ||
| const versions = {}; | ||
| let checkVersion = false; |
There was a problem hiding this comment.
I think checkVersion is basically versionNumbers's truthiness.
There was a problem hiding this comment.
Yeah, taking a second look I agree.
|
So I tried a few different versions. Ultimately the Set was the most consistent and fastest, results from the 3 different tests are below: Object InSethasOwnProperty |
|
Nice :D |
|
Perfect, I'll be waiting for your update with a Set then. FYI I have noticed the same kind of improvement using Maps instead of objects as hash, I might be switching joi to that where it makes sense at some point. |
|
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
This PR includes some performance improvements to the run-time validation of the
string().guid()validation type.How Was This Determined?
I ran a series of benchmarks for some common cases of GUID/UUIDs and then made tweaks to improve the performance.
Schemas
Benchmarks
Original Benchmarks
Unmodified I received the following benchmarks:
From there I deferred more of the work to the Regular Expression but kept the version check within an
ifstatement, which brought me the following results:From there I went and pulled the version &
89ABchecks along with utilizing back references within the Regular Expression for divider checks. Additionally, I simplified the Regular Expression to eliminate capture & non-capture groups where possible. This lead to the follow (current) benchmarks: