Use Set() for internal Set backend #1415
Conversation
|
Actually, this might not be the best way forward, as it seems to slow down the actual validation. |
|
I have updated the PR with another performance patch, which increases the test case validation speed by ~50x compared to the current code. I'd say this is enough to warrant the optimisation. |
There was a problem hiding this comment.
Thanks, this is the path I wanted to explore and it looks a lot like what I'd have done. One of the problem I have with this are the for..of though, this translates to horrible things in babel, I can probably take care of that in another commit, however you prefer.
lib/set.js
Outdated
| return false; | ||
| } | ||
|
|
||
| const isSimpleValue = !(value instanceof Date) && !(insensitive && typeof value === 'string') && !Buffer.isBuffer(value); |
There was a problem hiding this comment.
Wouldn't it be simpler to go directly for a .has test ? I don't think there's any case where it would give false positives if true.
There was a problem hiding this comment.
It's simpler for sure, and should work. It might reduce the performance slightly for some cases, though it will speed up others tremendously. Eg. insensitive string lookup, where it actually matches the case.
I expect it will be a net win.
|
FYI I'm looking into a cleaner implementation along with further performance enhancements to the slow path. It's done, but missing a bunch of tests to get 100% coverage. |
* Always check if value is found by a simple lookup * Flags set as "dirty" whenever a ref is added to avoid manual iteration (speed boost when missing value) * Precomputes non-simple value check to avoid redundant comparisons in iteration * Only allow refs to contain array items (also a bug fix)
| (Buffer.isBuffer(value) && Buffer.isBuffer(item) && value.length === item.length && value.toString('binary') === item.toString('binary'))) { | ||
| for (let item of this._set) { | ||
| if (checkRef && Ref.isRef(item)) { // Only resolve references if there is a state, otherwise it's a merge | ||
| item = item(state.reference || state.parent, options); |
There was a problem hiding this comment.
The missing coverage is because state.reference is always falsy.
I'm not sure how to add a test that exercises this code. Any ideas?
|
I have benchmarked this code a bit, and it seems to perform the same or better (up to ~1000x) in all cases. Both when constructing schemas, and when validating. |
|
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. |
Along with a
isSimpleValueoptimisation, this provides a 1000x speedup for the case in #1409.