Skip to content
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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 5, 2021
Merged

Perf of dereference is unstable #17

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
55dee7e
Perf of dereference is unstable
P0lip Feb 3, 2021
75b75a4
Merge branch 'master' into slower-dereference
P0lip Mar 3, 2021
2f49fe6
chore: removed wrong comment
P0lip Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 35 additions & 40 deletions lib/dereference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module.exports = dereference;
*/
function dereference (parser, options) {
// console.log('Dereferencing $ref pointers in %s', parser.$refs._root$Ref.path);
let dereferenced = crawl(parser.schema, parser.$refs._root$Ref.path, "#", [], [], {}, parser.$refs, options);
let dereferenced = crawl(parser.schema, parser.$refs._root$Ref.path, "#", [], {}, parser.$refs, options);
parser.$refs.circular = dereferenced.circular;
parser.schema = dereferenced.value;
}
Expand All @@ -28,65 +28,61 @@ function dereference (parser, options) {
* @param {string} path - The full path of `obj`, possibly with a JSON Pointer in the hash
* @param {string} pathFromRoot - The path of `obj` from the schema root
* @param {object[]} parents - An array of the parent objects that have already been dereferenced
* @param {object[]} processedObjects - An array of all the objects that have already been processed
* @param {object} dereferencedCache - An map of all the dereferenced objects
* @param {$Refs} $refs
* @param {$RefParserOptions} options
* @returns {{value: object, circular: boolean}}
*/
function crawl (obj, path, pathFromRoot, parents, processedObjects, dereferencedCache, $refs, options) {
function crawl (obj, path, pathFromRoot, parents, dereferencedCache, $refs, options) {
let dereferenced;
let result = {
value: obj,
circular: false
};

if (options.dereference.circular === "ignore" || processedObjects.indexOf(obj) === -1) {
Copy link

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?

Copy link
Author

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)

Copy link
Author

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.

if (obj && typeof obj === "object" && !ArrayBuffer.isView(obj)) {
parents.push(obj);
processedObjects.push(obj);
if (obj && typeof obj === "object" && !ArrayBuffer.isView(obj)) {
parents.push(obj);

if ($Ref.isAllowed$Ref(obj, options)) {
dereferenced = dereference$Ref(obj, path, pathFromRoot, parents, processedObjects, dereferencedCache, $refs, options);
result.circular = dereferenced.circular;
result.value = dereferenced.value;
}
else {
for (let key of Object.keys(obj)) {
let keyPath = Pointer.join(path, key);
let keyPathFromRoot = Pointer.join(pathFromRoot, key);
let value = obj[key];
let circular = false;

if ($Ref.isAllowed$Ref(value, options)) {
dereferenced = dereference$Ref(value, keyPath, keyPathFromRoot, parents, processedObjects, dereferencedCache, $refs, options);
if ($Ref.isAllowed$Ref(obj, options)) {
dereferenced = dereference$Ref(obj, path, pathFromRoot, parents, dereferencedCache, $refs, options);
result.circular = dereferenced.circular;
result.value = dereferenced.value;
}
else {
for (let key of Object.keys(obj)) {
let keyPath = Pointer.join(path, key);
let keyPathFromRoot = Pointer.join(pathFromRoot, key);
let value = obj[key];
let circular = false;

if ($Ref.isAllowed$Ref(value, options)) {
dereferenced = dereference$Ref(value, keyPath, keyPathFromRoot, parents, dereferencedCache, $refs, options);
circular = dereferenced.circular;
// Avoid pointless mutations; breaks frozen objects to no profit
if (obj[key] !== dereferenced.value) {
obj[key] = dereferenced.value;
}
}
else {
if (parents.indexOf(value) === -1) {
dereferenced = crawl(value, keyPath, keyPathFromRoot, parents, dereferencedCache, $refs, options);
circular = dereferenced.circular;
// Avoid pointless mutations; breaks frozen objects to no profit
if (obj[key] !== dereferenced.value) {
obj[key] = dereferenced.value;
}
}
else {
if (parents.indexOf(value) === -1) {
dereferenced = crawl(value, keyPath, keyPathFromRoot, parents, processedObjects, dereferencedCache, $refs, options);
circular = dereferenced.circular;
// Avoid pointless mutations; breaks frozen objects to no profit
if (obj[key] !== dereferenced.value) {
obj[key] = dereferenced.value;
}
}
else {
circular = foundCircularReference(keyPath, $refs, options);
}
circular = foundCircularReference(keyPath, $refs, options);
}

// Set the "isCircular" flag if this or any other property is circular
result.circular = result.circular || circular;
}
}

parents.pop();
// Set the "isCircular" flag if this or any other property is circular
result.circular = result.circular || circular;
}
}

parents.pop();
}

return result;
Expand All @@ -104,7 +100,7 @@ function crawl (obj, path, pathFromRoot, parents, processedObjects, dereferenced
* @param {$RefParserOptions} options
* @returns {{value: object, circular: boolean}}
*/
function dereference$Ref ($ref, path, pathFromRoot, parents, processedObjects, dereferencedCache, $refs, options) {
function dereference$Ref ($ref, path, pathFromRoot, parents, dereferencedCache, $refs, options) {
P0lip marked this conversation as resolved.
Show resolved Hide resolved
// console.log('Dereferencing $ref pointer "%s" at %s', $ref.$ref, path);

let $refPath = url.resolve(path, $ref.$ref);
Expand All @@ -129,7 +125,6 @@ function dereference$Ref ($ref, path, pathFromRoot, parents, processedObjects, d
return cache;
}


let pointer = $refs._resolve($refPath, path, options);

if (pointer === null) {
Expand All @@ -150,7 +145,7 @@ function dereference$Ref ($ref, path, pathFromRoot, parents, processedObjects, d
// Crawl the dereferenced value (unless it's circular)
if (!circular) {
// Determine if the dereferenced value is circular
let dereferenced = crawl(dereferencedValue, pointer.path, pathFromRoot, parents, processedObjects, dereferencedCache, $refs, options);
let dereferenced = crawl(dereferencedValue, pointer.path, pathFromRoot, parents, dereferencedCache, $refs, options);
circular = dereferenced.circular;
dereferencedValue = dereferenced.value;
}
Expand Down