-
-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: optimize traversals with a very large number of errors #1172
Conversation
Possibly I forgot the |
feccc9e
to
ecc6025
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not optimized enough for an optimization PR
ark/schema/shared/traversal.ts
Outdated
const propString = pathToPropString(path) | ||
return this.errors.some(e => propString.startsWith(e.propString)) | ||
for (let i = 0; i <= path.length; i++) { | ||
const partialPropString = pathToPropString(path.slice(0, i)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pathToPropString
already has to iterate the path. Look at that logic and reuse the internal methods so build it here as you go so this isn't n^2
ark/schema/shared/traversal.ts
Outdated
const propString = pathToPropString(path) | ||
return this.errors.some(e => propString.startsWith(e.propString)) | ||
let partialPropString: string = "" | ||
if (Object.hasOwn(this.errors.byPath, partialPropString)) return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to use in
and then change errorsByPath
to initialize to Object.create(null)
?
You are right to consider that edge case but I think solving it in the places where we're using records is better, see #1121
export const appendPropToPathString: AppendPropToPathStringFn = ( | ||
path, | ||
prop, | ||
...[opts] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be opts
without TS complaining? The signature there is really just a TS trick to force opts to be passed in some situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not without any
s
It's possible to make it a function
with overloads and loose implementation though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I can make it required here, as it's internal anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that didn't work
No description provided.