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.

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

Avoid allocation for normalized absolute paths. #60755

Closed
wants to merge 12 commits into from
18 changes: 13 additions & 5 deletions src/compiler/builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ import {
isNumber,
isString,
map,
mapDefined,
mapDefinedIterator,
maybeBind,
noop,
Expand Down Expand Up @@ -1236,7 +1237,7 @@ function getBuildInfo(state: BuilderProgramStateWithDefinedProgram): BuildInfo {
const currentDirectory = state.program.getCurrentDirectory();
const buildInfoDirectory = getDirectoryPath(getNormalizedAbsolutePath(getTsBuildInfoEmitOutputFilePath(state.compilerOptions)!, currentDirectory));
// Convert the file name to Path here if we set the fileName instead to optimize multiple d.ts file emits and having to compute Canonical path
const latestChangedDtsFile = state.latestChangedDtsFile ? relativeToBuildInfoEnsuringAbsolutePath(state.latestChangedDtsFile) : undefined;
const latestChangedDtsFile = state.latestChangedDtsFile ? relativePathToBuildInfo(state.latestChangedDtsFile) : undefined;
const fileNames: string[] = [];
const fileNameToFileId = new Map<string, IncrementalBuildInfoFileId>();
const rootFileNames = new Set(state.program.getRootFileNames().map(f => toPath(f, currentDirectory, state.program.getCanonicalFileName)));
Expand Down Expand Up @@ -1377,10 +1378,17 @@ function getBuildInfo(state: BuilderProgramStateWithDefinedProgram): BuildInfo {
};
return buildInfo;

function relativeToBuildInfoEnsuringAbsolutePath(path: string) {
function relativePathToBuildInfo(path: string) {
return relativeToBuildInfo(getNormalizedAbsolutePath(path, currentDirectory));
}

function relativePathToBuildInfoOrOriginalValue(path: unknown): string | undefined {
if (typeof path === "string") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure what to do here. Our options parser does discard invalid values. I wanted to make the most minimal change to builder.ts on this one.

return relativePathToBuildInfo(path);
}
return undefined;
}

function relativeToBuildInfo(path: string) {
return ensurePathIsNonModuleName(getRelativePathFromDirectory(buildInfoDirectory, path, state.program.getCanonicalFileName));
}
Expand Down Expand Up @@ -1457,13 +1465,13 @@ function getBuildInfo(state: BuilderProgramStateWithDefinedProgram): BuildInfo {
if (option) {
Debug.assert(option.type !== "listOrElement");
if (option.type === "list") {
const values = value as readonly string[];
const values = value as readonly unknown[];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that this type assertion probably isn't correct, and we need to validate that value actually is an array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get this to fail by writing a test.

if (option.element.isFilePath && values.length) {
return values.map(relativeToBuildInfoEnsuringAbsolutePath);
return mapDefined(values, relativePathToBuildInfoOrOriginalValue);
}
}
else if (option.isFilePath) {
return relativeToBuildInfoEnsuringAbsolutePath(value as string);
return relativePathToBuildInfoOrOriginalValue(value);
}
}
return value;
Expand Down
4 changes: 3 additions & 1 deletion src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,9 @@ function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOpt
return pathsOnly ? undefined : relativePath;
}

const baseDirectory = getNormalizedAbsolutePath(getPathsBasePath(compilerOptions, host) || baseUrl!, host.getCurrentDirectory());
const currentDirectory = host.getCurrentDirectory();
const basePath = getPathsBasePath(compilerOptions, host) ?? baseUrl ?? currentDirectory;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We incorrectly assumed baseUrl would be defined; however, the user could incorrectly have set paths and not baseUrl.

const baseDirectory = getNormalizedAbsolutePath(basePath, currentDirectory);
const relativeToBaseUrl = getRelativePathIfInSameVolume(moduleFileName, baseDirectory, getCanonicalFileName);
if (!relativeToBaseUrl) {
return pathsOnly ? undefined : relativePath;
Expand Down
23 changes: 22 additions & 1 deletion src/compiler/path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,28 @@ export function getNormalizedPathComponents(path: string, currentDirectory: stri

/** @internal */
export function getNormalizedAbsolutePath(fileName: string, currentDirectory: string | undefined): string {
return getPathFromPathComponents(getNormalizedPathComponents(fileName, currentDirectory));
fileName = normalizeSlashes(fileName);

if (isNotNormalizedOrAbsolute(fileName)) {
return getPathFromPathComponents(getNormalizedPathComponents(fileName, currentDirectory));
Comment on lines +630 to +631
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If fileName is already absolute, we could just call normalizePath on it, which has already been optimized.

I also wonder about both getNormalizedAbsolutePath and normalizePath, whether it’s better to index through the string, building a new one if needed, rather than doing the two array allocations involved in getPathFromPathComponents(reducePathComponents(getPathComponents(path)))?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely agree on that part, I think that iterating through each non-normalized component would be way better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrator: it was way better

image

}

return fileName;
}

function isNotNormalizedOrAbsolute(s: string) {
if (getEncodedRootLength(s) === 0) {
// An absolute path must have a root component.
return true;
}

const lastChar = s.charCodeAt(s.length - 1);
if (lastChar === CharacterCodes.slash) {
// A normalized path cannot end in a trailing separator.
return true;
}

return relativePathSegmentRegExp.test(s);
}

/** @internal */
Expand Down
2 changes: 1 addition & 1 deletion src/services/stringCompletions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,7 @@ function getCompletionEntriesForNonRelativeModules(
}

if (paths) {
const absolute = getPathsBasePath(compilerOptions, host)!;
const absolute = getPathsBasePath(compilerOptions, host)!; // Always defined when 'paths' is defined
addCompletionEntriesFromPaths(result, fragment, absolute, extensionOptions, program, host, moduleSpecifierResolutionHost, paths);
}

Expand Down
104 changes: 104 additions & 0 deletions src/testRunner/unittests/paths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,110 @@ describe("unittests:: core paths", () => {
assert.strictEqual(ts.resolvePath("a", "b", "/c"), "/c");
assert.strictEqual(ts.resolvePath("a", "b", "../c"), "a/c");
});
it("getNormalizedAbsolutePath", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised #60802 hasn't made this diff go away, maybe needs a main merge?

assert.strictEqual(ts.getNormalizedAbsolutePath("/", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/.", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/./", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/../", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a", ""), "/a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/", ""), "/a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/.", ""), "/a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/foo.", ""), "/a/foo.");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/./", ""), "/a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/./b", ""), "/a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/./b/", ""), "/a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/..", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/../", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/../", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/../b", ""), "/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/../b/", ""), "/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/..", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/..", "/"), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/..", "b/"), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/..", "/b"), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/.", "b"), "/a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/.", "."), "/a");

// Tests as above, but with backslashes.
assert.strictEqual(ts.getNormalizedAbsolutePath("\\", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\.", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\.\\", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\..\\", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\.\\", ""), "/a");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\.\\b", ""), "/a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\.\\b\\", ""), "/a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..\\", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..\\", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..\\b", ""), "/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..\\b\\", ""), "/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..", ""), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..", "\\"), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..", "b\\"), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\..", "\\b"), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\.", "b"), "/a");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\.", "."), "/a");

// Relative paths on an empty currentDirectory.
assert.strictEqual(ts.getNormalizedAbsolutePath("", ""), "");
assert.strictEqual(ts.getNormalizedAbsolutePath(".", ""), "");
assert.strictEqual(ts.getNormalizedAbsolutePath("./", ""), "");
// Strangely, these do not normalize to the empty string.
assert.strictEqual(ts.getNormalizedAbsolutePath("..", ""), "..");
assert.strictEqual(ts.getNormalizedAbsolutePath("../", ""), "..");

// Interaction between relative paths and currentDirectory.
assert.strictEqual(ts.getNormalizedAbsolutePath("", "/home"), "/home");
assert.strictEqual(ts.getNormalizedAbsolutePath(".", "/home"), "/home");
assert.strictEqual(ts.getNormalizedAbsolutePath("./", "/home"), "/home");
assert.strictEqual(ts.getNormalizedAbsolutePath("..", "/home"), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("../", "/home"), "/");
assert.strictEqual(ts.getNormalizedAbsolutePath("a", "b"), "b/a");
assert.strictEqual(ts.getNormalizedAbsolutePath("a", "b/c"), "b/c/a");

// Base names starting or ending with a dot do not affect normalization.
assert.strictEqual(ts.getNormalizedAbsolutePath(".a", ""), ".a");
assert.strictEqual(ts.getNormalizedAbsolutePath("..a", ""), "..a");
assert.strictEqual(ts.getNormalizedAbsolutePath("a.", ""), "a.");
assert.strictEqual(ts.getNormalizedAbsolutePath("a..", ""), "a..");

assert.strictEqual(ts.getNormalizedAbsolutePath("/base/./.a", ""), "/base/.a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/../.a", ""), "/.a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/./..a", ""), "/base/..a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/../..a", ""), "/..a");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/./..a/b", ""), "/base/..a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/../..a/b", ""), "/..a/b");

assert.strictEqual(ts.getNormalizedAbsolutePath("/base/./a.", ""), "/base/a.");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/../a.", ""), "/a.");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/./a..", ""), "/base/a..");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/../a..", ""), "/a..");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/./a../b", ""), "/base/a../b");
assert.strictEqual(ts.getNormalizedAbsolutePath("/base/../a../b", ""), "/a../b");

// Consecutive intermediate slashes are normalized to a single slash.
assert.strictEqual(ts.getNormalizedAbsolutePath("a//b", ""), "a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("a///b", ""), "a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("a/b//c", ""), "a/b/c");
assert.strictEqual(ts.getNormalizedAbsolutePath("/a/b//c", ""), "/a/b/c");
assert.strictEqual(ts.getNormalizedAbsolutePath("//a/b//c", ""), "//a/b/c");

// Backslashes are converted to slashes,
// and then consecutive intermediate slashes are normalized to a single slash
assert.strictEqual(ts.getNormalizedAbsolutePath("a\\\\b", ""), "a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("a\\\\\\b", ""), "a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("a\\b\\\\c", ""), "a/b/c");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\b\\\\c", ""), "/a/b/c");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\\\a\\b\\\\c", ""), "//a/b/c");

// The same occurs for mixed slashes.
assert.strictEqual(ts.getNormalizedAbsolutePath("a/\\b", ""), "a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("a\\/b", ""), "a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("a\\/\\b", ""), "a/b");
assert.strictEqual(ts.getNormalizedAbsolutePath("a\\b//c", ""), "a/b/c");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\a\\b\\\\c", ""), "/a/b/c");
assert.strictEqual(ts.getNormalizedAbsolutePath("\\\\a\\b\\\\c", ""), "//a/b/c");
});
it("getPathRelativeTo", () => {
assert.strictEqual(ts.getRelativePathFromDirectory("/", "/", /*ignoreCase*/ false), "");
assert.strictEqual(ts.getRelativePathFromDirectory("/a", "/a", /*ignoreCase*/ false), "");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export = x;


//// [/home/src/workspaces/solution/common/tsconfig.tsbuildinfo]
{"fileNames":["../../../tslibs/ts/lib/lib.d.ts","./obj.json","./index.ts"],"fileIdsList":[[2]],"fileInfos":[{"version":"-32082413277-/// <reference no-default-lib=\"true\"/>\ninterface Boolean {}\ninterface Function {}\ninterface CallableFunction {}\ninterface NewableFunction {}\ninterface IArguments {}\ninterface Number { toExponential: any; }\ninterface Object {}\ninterface RegExp {}\ninterface String { charAt: any; }\ninterface Array<T> { length: number; [n: number]: T; }\ninterface ReadonlyArray<T> {}\ndeclare const console: { log(msg: any): void; };\ninterface SymbolConstructor {\n readonly species: symbol;\n readonly toStringTag: symbol;\n}\ndeclare var Symbol: SymbolConstructor;\ninterface Symbol {\n readonly [Symbol.toStringTag]: string;\n}\n","affectsGlobalScope":true},"2353615672-{\n \"val\": 42\n}","-5032674136-import x = require(\"./obj.json\");\nexport = x;\n"],"root":[2,3],"options":{"allowJs":true,"checkJs":true,"composite":true,"declaration":true,"esModuleInterop":true,"outDir":"..","rootDir":"..","skipLibCheck":true},"referencedMap":[[3,1]],"latestChangedDtsFile":"./index.d.ts","version":"FakeTSVersion"}
{"fileNames":["../../../tslibs/ts/lib/lib.d.ts","./obj.json","./index.ts"],"fileIdsList":[[2]],"fileInfos":[{"version":"-32082413277-/// <reference no-default-lib=\"true\"/>\ninterface Boolean {}\ninterface Function {}\ninterface CallableFunction {}\ninterface NewableFunction {}\ninterface IArguments {}\ninterface Number { toExponential: any; }\ninterface Object {}\ninterface RegExp {}\ninterface String { charAt: any; }\ninterface Array<T> { length: number; [n: number]: T; }\ninterface ReadonlyArray<T> {}\ndeclare const console: { log(msg: any): void; };\ninterface SymbolConstructor {\n readonly species: symbol;\n readonly toStringTag: symbol;\n}\ndeclare var Symbol: SymbolConstructor;\ninterface Symbol {\n readonly [Symbol.toStringTag]: string;\n}\n","affectsGlobalScope":true},"2353615672-{\n \"val\": 42\n}","-5032674136-import x = require(\"./obj.json\");\nexport = x;\n"],"root":[2,3],"options":{"allowJs":true,"checkJs":true,"composite":true,"declaration":true,"esModuleInterop":true,"rootDir":"..","skipLibCheck":true},"referencedMap":[[3,1]],"latestChangedDtsFile":"./index.d.ts","version":"FakeTSVersion"}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look closely, the real diff is something like

- "outDir":"..","rootDir":".."
+ "rootDir":".."

This is actually correct. Previously, we would try to normalize the path, possibly resolving against the current working directory as a base. combinePaths is very lax about non-string trailing values, so we'd end up with combinePaths(currentDirectory, null) turning into currentDirectory. So rather than respect the fact that the outDir was actually being unset, we would set the outDir to the relative path from the configuration file to the currentDirectory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readable format is below in the diff, FWIW


//// [/home/src/workspaces/solution/common/tsconfig.tsbuildinfo.readable.baseline.txt]
{
Expand Down Expand Up @@ -193,7 +193,6 @@ export = x;
"composite": true,
"declaration": true,
"esModuleInterop": true,
"outDir": "..",
"rootDir": "..",
"skipLibCheck": true
},
Expand All @@ -204,7 +203,7 @@ export = x;
},
"latestChangedDtsFile": "./index.d.ts",
"version": "FakeTSVersion",
"size": 1148
"size": 1134
}

//// [/home/src/workspaces/out/sub-project/index.js]
Expand Down
Loading