From 5995af25a37e15247d624878cebc9da2cd771f24 Mon Sep 17 00:00:00 2001 From: Christopher Chedeau Date: Tue, 18 Apr 2017 08:39:47 -0700 Subject: [PATCH] Bail when traversing === groups (#1294) This is the second part of the fix for the performance regression seen in #1250. In #1217, for correctness reasons, we're now traversing all the conditional groups. This means that we're now in O(n^2). But, in practice, many of those groups are === between each others. So we only need to recurse through one of the instances to know if it's going to break. This makes the first example go from not terminating to being instant. The second one going from not terminating to taking ~1s. We can also make it instant by tweaking the printing phase, but that's for another PR. --- src/doc-utils.js | 54 +++-- .../__snapshots__/jsfmt.spec.js.snap | 216 ++++++++++++++++++ tests/performance/jsfmt.spec.js | 1 + tests/performance/nested-real.js | 83 +++++++ tests/performance/nested.js | 19 ++ 5 files changed, 351 insertions(+), 22 deletions(-) create mode 100644 tests/performance/__snapshots__/jsfmt.spec.js.snap create mode 100644 tests/performance/jsfmt.spec.js create mode 100644 tests/performance/nested-real.js create mode 100644 tests/performance/nested.js diff --git a/src/doc-utils.js b/src/doc-utils.js index f9ed9442..94b88ce9 100644 --- a/src/doc-utils.js +++ b/src/doc-utils.js @@ -1,34 +1,35 @@ "use strict"; function traverseDoc(doc, onEnter, onExit, shouldTraverseConditionalGroups) { - var hasStopped = false; function traverseDocRec(doc) { + var shouldRecurse = true; if (onEnter) { - hasStopped = hasStopped || onEnter(doc) === false; - } - if (hasStopped) { - return; + if (onEnter(doc) === false) { + shouldRecurse = false; + } } - if (doc.type === "concat") { - for (var i = 0; i < doc.parts.length; i++) { - traverseDocRec(doc.parts[i]); - } - } else if (doc.type === "if-break") { - if (doc.breakContents) { - traverseDocRec(doc.breakContents); - } - if (doc.flatContents) { - traverseDocRec(doc.flatContents); - } - } else if (doc.type === "group" && doc.expandedStates) { - if (shouldTraverseConditionalGroups) { - doc.expandedStates.forEach(traverseDocRec); - } else { + if (shouldRecurse) { + if (doc.type === "concat") { + for (var i = 0; i < doc.parts.length; i++) { + traverseDocRec(doc.parts[i]); + } + } else if (doc.type === "if-break") { + if (doc.breakContents) { + traverseDocRec(doc.breakContents); + } + if (doc.flatContents) { + traverseDocRec(doc.flatContents); + } + } else if (doc.type === "group" && doc.expandedStates) { + if (shouldTraverseConditionalGroups) { + doc.expandedStates.forEach(traverseDocRec); + } else { + traverseDocRec(doc.contents); + } + } else if (doc.contents) { traverseDocRec(doc.contents); } - } else if (doc.contents) { - traverseDocRec(doc.contents); } if (onExit) { @@ -60,10 +61,14 @@ function mapDoc(doc, func) { function findInDoc(doc, fn, defaultValue) { var result = defaultValue; + var hasStopped = false; traverseDoc(doc, function(doc) { var maybeResult = fn(doc); if (maybeResult !== undefined) { + hasStopped = true; result = maybeResult; + } + if (hasStopped) { return false; } }); @@ -120,6 +125,7 @@ function breakParentGroup(groupStack) { } function propagateBreaks(doc) { + var alreadyVisited = new Map(); const groupStack = []; traverseDoc( doc, @@ -129,6 +135,10 @@ function propagateBreaks(doc) { } if (doc.type === "group") { groupStack.push(doc); + if (alreadyVisited.has(doc)) { + return false; + } + alreadyVisited.set(doc, true); } }, doc => { diff --git a/tests/performance/__snapshots__/jsfmt.spec.js.snap b/tests/performance/__snapshots__/jsfmt.spec.js.snap new file mode 100644 index 00000000..f7a9c43e --- /dev/null +++ b/tests/performance/__snapshots__/jsfmt.spec.js.snap @@ -0,0 +1,216 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`nested.js 1`] = ` +someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + anotherFunction(); + }); + }); + }); + }); + }); + }); + }); + }); +}); +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + anotherFunction(); + }); + }); + }); + }); + }); + }); + }); + }); +}); + +`; + +exports[`nested-real.js 1`] = ` +tap.test("RecordImport.advance", (t) => { + const checkStates = (batches, states) => { + t.equal(batches.length, states.length); + for (const batch of batches) { + t.equal(batch.state, states.shift()); + t.ok(batch.getCurState().name(i18n)); + } + }; + + const batch = init.getRecordBatch(); + const dataFile = path.resolve(process.cwd(), "testData", "default.json"); + + const getBatches = (callback) => { + RecordImport.find({}, "", {}, (err, batches) => { + callback(null, batches.filter((batch) => (batch.state !== "error" && + batch.state !== "completed"))); + }); + }; + + mockFS((callback) => { + batch.setResults([fs.createReadStream(dataFile)], (err) => { + t.error(err, "Error should be empty."); + t.equal(batch.results.length, 6, "Check number of results"); + for (const result of batch.results) { + t.equal(result.result, "unknown"); + t.ok(result.data); + t.equal(result.data.lang, "en"); + } + + getBatches((err, batches) => { + checkStates(batches, ["started"]); + + RecordImport.advance((err) => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, ["process.completed"]); + + // Need to manually move to the next step + batch.importRecords((err) => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, ["import.completed"]); + + RecordImport.advance((err) => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, + ["similarity.sync.completed"]); + + RecordImport.advance((err) => { + t.error(err, + "Error should be empty."); + + t.ok(batch.getCurState() + .name(i18n)); + + getBatches((err, batches) => { + checkStates(batches, []); + t.end(); + callback(); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + }); +}); +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +tap.test("RecordImport.advance", t => { + const checkStates = (batches, states) => { + t.equal(batches.length, states.length); + for (const batch of batches) { + t.equal(batch.state, states.shift()); + t.ok(batch.getCurState().name(i18n)); + } + }; + + const batch = init.getRecordBatch(); + const dataFile = path.resolve(process.cwd(), "testData", "default.json"); + + const getBatches = callback => { + RecordImport.find({}, "", {}, (err, batches) => { + callback( + null, + batches.filter( + batch => batch.state !== "error" && batch.state !== "completed" + ) + ); + }); + }; + + mockFS(callback => { + batch.setResults([fs.createReadStream(dataFile)], err => { + t.error(err, "Error should be empty."); + t.equal(batch.results.length, 6, "Check number of results"); + for (const result of batch.results) { + t.equal(result.result, "unknown"); + t.ok(result.data); + t.equal(result.data.lang, "en"); + } + + getBatches((err, batches) => { + checkStates(batches, ["started"]); + + RecordImport.advance(err => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, ["process.completed"]); + + // Need to manually move to the next step + batch.importRecords(err => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, ["import.completed"]); + + RecordImport.advance(err => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, ["similarity.sync.completed"]); + + RecordImport.advance(err => { + t.error(err, "Error should be empty."); + + t.ok(batch.getCurState().name(i18n)); + + getBatches((err, batches) => { + checkStates(batches, []); + t.end(); + callback(); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + }); +}); + +`; diff --git a/tests/performance/jsfmt.spec.js b/tests/performance/jsfmt.spec.js new file mode 100644 index 00000000..989047bc --- /dev/null +++ b/tests/performance/jsfmt.spec.js @@ -0,0 +1 @@ +run_spec(__dirname); diff --git a/tests/performance/nested-real.js b/tests/performance/nested-real.js new file mode 100644 index 00000000..c69ec8dc --- /dev/null +++ b/tests/performance/nested-real.js @@ -0,0 +1,83 @@ +tap.test("RecordImport.advance", (t) => { + const checkStates = (batches, states) => { + t.equal(batches.length, states.length); + for (const batch of batches) { + t.equal(batch.state, states.shift()); + t.ok(batch.getCurState().name(i18n)); + } + }; + + const batch = init.getRecordBatch(); + const dataFile = path.resolve(process.cwd(), "testData", "default.json"); + + const getBatches = (callback) => { + RecordImport.find({}, "", {}, (err, batches) => { + callback(null, batches.filter((batch) => (batch.state !== "error" && + batch.state !== "completed"))); + }); + }; + + mockFS((callback) => { + batch.setResults([fs.createReadStream(dataFile)], (err) => { + t.error(err, "Error should be empty."); + t.equal(batch.results.length, 6, "Check number of results"); + for (const result of batch.results) { + t.equal(result.result, "unknown"); + t.ok(result.data); + t.equal(result.data.lang, "en"); + } + + getBatches((err, batches) => { + checkStates(batches, ["started"]); + + RecordImport.advance((err) => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, ["process.completed"]); + + // Need to manually move to the next step + batch.importRecords((err) => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, ["import.completed"]); + + RecordImport.advance((err) => { + t.error(err, "Error should be empty."); + + getBatches((err, batches) => { + checkStates(batches, + ["similarity.sync.completed"]); + + RecordImport.advance((err) => { + t.error(err, + "Error should be empty."); + + t.ok(batch.getCurState() + .name(i18n)); + + getBatches((err, batches) => { + checkStates(batches, []); + t.end(); + callback(); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + + t.ok(batch.getCurState().name(i18n)); + }); + }); + }); +}); diff --git a/tests/performance/nested.js b/tests/performance/nested.js new file mode 100644 index 00000000..8abe3588 --- /dev/null +++ b/tests/performance/nested.js @@ -0,0 +1,19 @@ +someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + return someObject.someFunction().then(function() { + anotherFunction(); + }); + }); + }); + }); + }); + }); + }); + }); +});