-
Notifications
You must be signed in to change notification settings - Fork 7
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 of dereference is unstable #17
Conversation
let dereferenced; | ||
let result = { | ||
value: obj, | ||
circular: false | ||
}; | ||
|
||
if (options.dereference.circular === "ignore" || processedObjects.indexOf(obj) === -1) { |
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.
Wait, so you fixed the performance by removing protection against circular references?
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.
Nope!
It's just a redundant check that didn't seem to have any actual impact on perf.
https://github.com/stoplightio/json-schema-ref-parser/blob/55dee7ed978b734a1fd354e01197236b3eaa106c/lib/dereference.js#L108L126 is what actually made impact. That added check didn't contribute.
For a context this is the change I'm talking about 5c3eece
I just didn't notice options.dereference.circular === "ignore" || processedObjects.indexOf(obj) === -1)
changing anything so I reverted to the initial version of this code (prior to the aforementioned commit)
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.
We may potentially just use a bit more conservative change that was implemented here APIDevTools#212
It only swapped arrays with sets, but this already mitigated the perf cliff.
https://github.com/stoplightio/platform-internal/issues/5517 reminded me about this |
@P0lip if https://github.com/stoplightio/platform-internal/issues/5517 is caused by this, can I assign that ticket to you guys? |
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.
It looks good. If it was a redundant check then can't complain about removing it :)
Just remove a jsdoc comment for processedObjects
, but it's approve from me 👍
See APIDevTools#211