Skip to content

Commit

Permalink
perf: only modify array once per action & write once per file
Browse files Browse the repository at this point in the history
On a file with 10000 of the same typo, replacing all of that typo will be ~8% faster and include 99.99% less reads and writes
  • Loading branch information
Lioness100 committed Feb 22, 2023
1 parent 7de31ed commit d329e83
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 43 deletions.
7 changes: 2 additions & 5 deletions src/display.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,23 +49,20 @@ export const centerText = (text: string, length: number, width: number) => {
};

export const determineAction = async (
url: URL,
issue: Issue,
issues: Issue[],
totalIssueCount: number
): Promise<[Action, string]> => {
const index = totalIssueCount - issues.length;
const progressIndicator = `${index}/${totalIssueCount} ── `;
const text = formatContext(issue);
const path = fileURLToPath(url);
const path = fileURLToPath(new URL(issue.uri!));
const trace = `:${issue.row}:${issue.col}`;

// Create a header that displays the absolute path to the file and the line and column of the typo. This header
// is centered in the terminal (the width of the terminal is stored in process.stdout.columns)

const typoLocation = bold(
greenBright(progressIndicator) + whiteBright(fileURLToPath(url)) + cyan(`:${issue.row}:${issue.col}`)
);
const typoLocation = bold(greenBright(progressIndicator) + whiteBright(path) + cyan(`:${issue.row}:${issue.col}`));

const width = process.stdout.columns;
const typoLocationHeader = centerText(typoLocation, path.length + trace.length + progressIndicator.length, width);
Expand Down
91 changes: 55 additions & 36 deletions src/handleIssue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,19 @@ import { addIgnoreWordToSettings, writeToSettings } from './config';

let totalIssueCount = 0;

export const writeChangeToFile = async (url: URL, issue: Issue, replacer: string) => {
const file = await readFile(url, 'utf8');

export const updateFileText = (issue: Issue, replacer: string, file: string) => {
// `issue.offset` is the index of the first character of the issue. With this, we slice everything in the file
// before the text, then add the replacer, then add everything after the text.
const newFile = file.slice(0, issue.offset) + replacer + file.slice(issue.offset + issue.text.length);
await writeFile(url, newFile);
return file.slice(0, issue.offset) + replacer + file.slice(issue.offset + issue.text.length);
};

export const replaceSingleChange = async (issue: Issue, issues: Issue[], replacer: string) => {
previousState.resolvedIssues.push(issue);
updateFutureIssues(issue, replacer, issues);

const url = new URL(issue.uri!);
const file = await readFile(url, 'utf8');
await writeFile(url, updateFileText(issue, replacer, file));
};

// When we replace a word, we need to update the offset of all future typos in that file, because they would be
Expand Down Expand Up @@ -66,15 +72,11 @@ export const updateFutureIssues = (issue: Issue, replacer: string, issues: Issue
}
};

export const fixIssue = async (issues: Issue[], issue: Issue, replacer: string, url: URL) => {
previousState.resolvedIssues.push(issue);
updateFutureIssues(issue, replacer, issues);
await writeChangeToFile(url, issue, replacer).catch(() => null);
};
export const performSideEffects = async (issues: Issue[], issue: Issue, action: Action, replacer: string) => {
const filesToWrite = new Map<string, { file: string; url: URL }>();
const updatedIssues = [];

export const performSideEffects = async (issues: Issue[], issue: Issue, action: Action, replacer: string, url: URL) => {
// The issues array is modified in place, so we need to make a copy of it.
for (const futureIssue of [...issues]) {
for (const [idx, futureIssue] of issues.entries()) {
if (
!futureIssue.uri ||
// If we're skipping the file, we only want to remove issues that share the same URI. Otherwise (action
Expand All @@ -83,22 +85,26 @@ export const performSideEffects = async (issues: Issue[], issue: Issue, action:
? futureIssue.uri !== issue.uri
: futureIssue.text !== issue.text
) {
updatedIssues.push(futureIssue);
continue;
}

issues.splice(issues.indexOf(futureIssue), 1);
if (action === Action.ReplaceAll) {
const url = new URL(futureIssue.uri!);
const file = filesToWrite.get(futureIssue.uri!)?.file ?? (await readFile(url, 'utf8'));
filesToWrite.set(futureIssue.uri!, { file: updateFileText(futureIssue, replacer, file), url });

// If we're ignoring the issue, we don't need to do any more work.
if (action !== Action.ReplaceAll) {
continue;
previousState.resolvedIssues.push(futureIssue);
updateFutureIssues(futureIssue, replacer, issues.slice(idx + 1));
}

// Otherwise, we update the offsets of future issues and write the change to file, just as we would if it
// was a normal Replace action.
// TODO: If multiple of these issues are in the same file, we should store the file contents in memory and
// only write it once at the end.
await fixIssue(issues, futureIssue, replacer, futureIssue.uri === issue.uri ? url : new URL(futureIssue.uri!));
}

// `issues.splice(0, issues.length, ...)` is used to replace the entire array in place. This is faster than calling
// issues.splice individually, especially when there are many issues.
issues.splice(0, issues.length, ...updatedIssues);

// Only write to a file once, instead of once per issue.
await Promise.allSettled([...filesToWrite.values()].map(({ url, file }) => writeFile(url, file)));
};

export const restoreState = async (issues: Issue[]) => {
Expand All @@ -109,12 +115,21 @@ export const restoreState = async (issues: Issue[]) => {
}

if (replacer) {
for (const resolvedIssue of resolvedIssues) {
// Replace the new replacer with the old text instead of the other way around.
const newReplacer = resolvedIssue.text;
resolvedIssue.text = replacer;
await writeChangeToFile(new URL(resolvedIssue.uri!), resolvedIssue, newReplacer).catch(() => null);
// Abridged version of performSideEffects, but without updating future issues, because we're restoring the state
// right after this.
const filesToWrite = new Map<string, { file: string; url: URL }>();
for (const resolvedIssue of resolvedIssues.reverse()) {
const url = new URL(resolvedIssue.uri!);
const file = filesToWrite.get(resolvedIssue.uri!)?.file ?? (await readFile(url, 'utf8'));

// The issue text is now the replacer and it should be replaced with the original text to effectively revert
// the change.
const newFile = updateFileText({ ...resolvedIssue, text: replacer }, resolvedIssue.text, file);

filesToWrite.set(resolvedIssue.uri!, { file: newFile, url });
}

await Promise.allSettled([...filesToWrite.values()].map(({ url, file }) => writeFile(url, file)));
}

// Edit the entire issues array in place, so that the reference is the same.
Expand All @@ -127,8 +142,7 @@ export const handleIssue = async (issues: Issue[], issue: Issue) => {
return;
}

const url = new URL(issue.uri);
const [action, replacer] = await determineAction(url, issue, issues, totalIssueCount);
const [action, replacer] = await determineAction(issue, issues, totalIssueCount);

previousState.action = action;

Expand All @@ -155,13 +169,18 @@ export const handleIssue = async (issues: Issue[], issue: Issue) => {
config: undefined
} as Omit<typeof previousState, 'action'>);

if (action === Action.Replace || action === Action.ReplaceAll) {
await fixIssue(issues, issue, replacer, url);
}
if (action === Action.Replace) {
await replaceSingleChange(issue, issues, replacer).catch(() => null);
} else if (action === Action.ReplaceAll || action === Action.IgnoreAll || action === Action.SkipFile) {
// The above actions all have side effects (as in, they require modification of future issues).

// Add the current issue back to the list of issues so it can be used in the performSideEffects loop to prevent
// duplicated code. It will be removed from the list in the loop.
if (action === Action.ReplaceAll) {
issues.unshift(issue);
}

// The following actions all have side effects (as in, they require modification of future issues).
if (action === Action.ReplaceAll || action === Action.IgnoreAll || action === Action.SkipFile) {
await performSideEffects(issues, issue, action, replacer, url);
await performSideEffects(issues, issue, action, replacer);

if (action === Action.IgnoreAll) {
await addIgnoreWordToSettings(issue.text).catch(() => null);
Expand Down
4 changes: 2 additions & 2 deletions test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ afterEach(() => {
});

describe.each(allTypoSets)('%s', (name, data) => {
test('Fixing & Displaying Typos', async () => {
test('should accurately find and replace individual typos', async () => {
file = data.text;

const originalIssueLength = data.issues.length;
Expand All @@ -52,7 +52,7 @@ describe.each(allTypoSets)('%s', (name, data) => {

// We can't use the original data.issues, because the `offset` and `line.text` properties are updated. So, we
// have to use the mocked `determineAction` function to get the issues that were displayed.
const displayedIssues = vi.mocked(determineAction).mock.calls.map(([, issue]) => issue);
const displayedIssues = vi.mocked(determineAction).mock.calls.map(([issue]) => issue);
const contextDisplays = displayedIssues.map((issue) => formatContext(issue));

expect(contextDisplays, name).toMatchObject(data.displays);
Expand Down

0 comments on commit d329e83

Please sign in to comment.