Skip to content

Commit

Permalink
Bail when traversing === groups (#1294)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@vjeux
vjeux committed Apr 18, 2017
1 parent 08e4e2c commit 5995af2
Show file tree
Hide file tree
Showing 5 changed files with 351 additions and 22 deletions.
54 changes: 32 additions & 22 deletions src/doc-utils.js
View file
@@ -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) {
Expand Down Expand Up @@ -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;
}
});
Expand Down Expand Up @@ -120,6 +125,7 @@ function breakParentGroup(groupStack) {
}

function propagateBreaks(doc) {
var alreadyVisited = new Map();
const groupStack = [];
traverseDoc(
doc,
Expand All @@ -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 => {
Expand Down
216 changes: 216 additions & 0 deletions tests/performance/__snapshots__/jsfmt.spec.js.snap
View file
@@ -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));
});
});
});
});
`;
1 change: 1 addition & 0 deletions tests/performance/jsfmt.spec.js
View file
@@ -0,0 +1 @@
run_spec(__dirname);
83 changes: 83 additions & 0 deletions tests/performance/nested-real.js
View file
@@ -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));
});
});
});
});

0 comments on commit 5995af2

Please sign in to comment.