For duplicate source files of the same package, make one redirect to the other #16274

ghost · 2017-06-05T21:58:02Z

Fixes #6496
When we resolve modules, we will now track what package name and version we loaded them from based on package.json.
If we see another package with an identical name and version, we will create a source file that redirects to the previously-created source file for that package.
We will still load in the underlying source file so that services can detect changes, but program.getSourceFileByPath will return the redirect, so imports to two different locations will return source files with the same content, meaning that any classes with privates will have the same identity.

…the other

ghost · 2017-06-06T15:47:02Z

On our baselines performance is the same, because they don't have duplicate packages and checking for them costs almost nothing.
I generated a sample project designed to have lots of duplication using the following script:

// @ts-check

const { existsSync,mkdirSync, writeFileSync } = require("fs");
const maxL = 50;
const maxX = 100;

main();

/** @return {void} */
function main() {
    writeFileSync("tsconfig.json", JSON.stringify({ "compilerOptions": { "noEmit": true } }));
    writeFileSync("src/index.ts", genIndex());

    ensureDir("node_modules");
    for (let li = 0; li < maxL; li++) {
        const ldir = `node_modules/l${li}`;
        ensureDir(ldir);
        writeFileSync(`${ldir}/index.d.ts`, genLIndex());

        ensureDir(`${ldir}/node_modules`);
        for (let xi = 0; xi < maxX; xi++) {
            const xdir = `${ldir}/node_modules/x${xi}`;
            ensureDir(xdir);
            writeFileSync(`${xdir}/index.d.ts`, `export class X${xi} { private x; }`);
            writeFileSync(`${xdir}/package.json`, JSON.stringify({ name: `x${xi}`, version: "1", }));
        }
    }
}

/** @return {string} */
function genIndex() {
    const lines = [];
    for (let li = 0; li < maxL; li++) {
        const imports = [];
        for (let xi = 0; xi < maxX; xi++) {
            imports.push(`X${xi} as L${li}X${xi}`);
        }
        lines.push(`import { ${imports.join(", ")} } from "l${li}";`);
    }

    lines.push(`let x = L0X0;`);
    for (let li = 1; li < maxL; li++) {
        lines.push(`x = L${li}X0;`);
    }

    return lines.join("\n");
}

/** @return {number} */
function genLIndex() {
    const lines = [];
    for (let xi = 0; xi < maxX; xi++) {
        lines.push(`export { X${xi} } from "x${xi}"`);
    }
    return lines.join("\n");
}

/**
 * @param {string} dirpath
 * @return {void}
 */
function ensureDir(dirPath) {
    if (existsSync(dirPath)) return;
    mkdirSync(dirPath);
}

I then compared tsc --diagnostics on master vs on this branch.

In master, there were 28004 types generated, and only 8404 on this branch, due to the deduplication.
Parse time and bind time are nearly identical. (The duplicate packages still get parsed.)
Memory used: 14% less (137201K -> 118066K)
Check time: 18.7% less (Fewer files checked, and also we there are no compile errors in this branch.)

After that I tested using Object.assign({}, redirectTo) instead of Object.create(redirectTo). It increased memory use by 28.5% (118065.75 -> 151612.5) and didn't affect total time. Based on this I would recommend continuing to use Object.create.

mhegazy · 2017-06-07T17:36:36Z

We also need to add some tests to src\harness\unittests\reuseProgramStructure.ts to ensure that we are not reusing after edits in cases like:

redirect was there but target changed
redirect was there but underlying changed
duplicate packageid change cause program structure invalidation

mhegazy · 2017-06-07T17:11:38Z

src/compiler/program.ts

                const newSourceFile = host.getSourceFileByPath
                    ? host.getSourceFileByPath(oldSourceFile.fileName, oldSourceFile.path, options.target)
                    : host.getSourceFile(oldSourceFile.fileName, options.target);

+                const packageId = oldProgram.getPackageIdOfSourceFile(oldSourceFile);
+                if (packageId) sourceFileToPackageId.set(newSourceFile.path, packageId);


i would move this to the end, when we are sure that we are completely reusing the program.

mhegazy · 2017-06-07T17:14:51Z

src/compiler/program.ts

@@ -765,11 +775,42 @@ namespace ts {
            const modifiedSourceFiles: { oldFile: SourceFile, newFile: SourceFile }[] = [];
            oldProgram.structureIsReused = StructureIsReused.Completely;

-            for (const oldSourceFile of oldProgram.getSourceFiles()) {
+            const oldSourceFiles = oldProgram.getSourceFiles();
+            for (const oldSourceFile of oldSourceFiles) {
                const newSourceFile = host.getSourceFileByPath
                    ? host.getSourceFileByPath(oldSourceFile.fileName, oldSourceFile.path, options.target)
                    : host.getSourceFile(oldSourceFile.fileName, options.target);


we should assert here that the sourceFile we got from the host never has a redirect set.

mhegazy · 2017-06-07T17:17:26Z

src/compiler/program.ts

+                        continue;
+                    }
+                }
+                else if (oldProgram.sourceFileIsRedirectedTo(oldSourceFile)) {


isSourceFileTargetOfRedirect ?

mhegazy · 2017-06-07T17:18:26Z

src/compiler/program.ts

+                        return oldProgram.structureIsReused = StructureIsReused.Not;
+                    }
+                    else {
+                        sourceFileIsRedirectedToSet.set(oldSourceFile.path, true);


probably want to move this to the end, and just copy them all instead of one by one.

mhegazy · 2017-06-07T17:19:31Z

src/compiler/program.ts

+                        sourceFileIsRedirectedToSet.set(oldSourceFile.path, true);
+                        filePaths.push(oldSourceFile.path);
+                        newSourceFiles.push(oldSourceFile);
+                        continue;


i would let this fall through, and then we will add it to filePaths, and newSoruceFiles

mhegazy · 2017-06-07T17:23:09Z

src/compiler/program.ts

+                    if (packageId) {
+                        // If this has a package id, and some other source file has the same package id, they are candidates to become redirects.
+                        // In that case we must rebuild the program.
+                        const hasDuplicate = oldSourceFiles.some(o => {


consider putting this in a map.

mhegazy · 2017-06-07T17:26:12Z

src/compiler/program.ts

@@ -1517,8 +1574,28 @@ namespace ts {
            }
        }

+        function createRedirectSourceFile(redirectTo: SourceFile, underlying: SourceFile, fileName: string, path: Path): SourceFile {
+            sourceFileIsRedirectedToSet.set(redirectTo.path, true);


consider putting all the map setting in this function.

On second thought, it would be better to do all of that outside since it makes more sense in the context of findSourceFile.

…if we completely reuse old structure

ghost · 2017-06-07T21:51:22Z

@mhegazy Comments should all be resolved now.

amcasey · 2017-07-11T17:44:30Z

It seems like another possible approach would be to dirty projects referencing a file sharing the same package ID as a modified file when the file watcher for that file fires. Would that allow greater structure reuse?

amcasey

Apparently, comments stay pending until I "submit".

amcasey · 2017-07-10T22:13:23Z

src/compiler/program.ts

@@ -1551,8 +1597,26 @@ namespace ts {
            }
        }

+        function createRedirectSourceFile(redirectTo: SourceFile, underlying: SourceFile, fileName: string, path: Path): SourceFile {


Can you please add some comments about the various SourceFiles involved? I think redirectTo is pointed to by the link and the result does the pointing. I'm not sure what underlying is. Maybe it's the file we would use if we weren't redirecting? If so, maybe "redirectionTarget" and "unredirected" would be clearer?

amcasey · 2017-07-10T23:56:52Z

src/compiler/program.ts

+                    // This lets us know if the underlying file has changed. If it has we should break the redirect.
+                    if (newSourceFile !== oldSourceFile.redirect.underlying) {
+                        // Underlying file has changed. Might not redirect anymore. Must rebuild program.
+                        return oldProgram.structureIsReused = StructureIsReused.Not;


SafeModules isn't conservative enough?

Oh, because we haven't set the dirty bit on the affected projects? Couldn't we? We're watching the files anyway, aren't we?

As far as I can tell, we use StructureIsReused.Not in situations where files are added or removed, which would include the case of redirects being broken or created.

Per our offline discussion, I agree that this is an appropriate way to handle changes.

amcasey · 2017-07-11T00:47:21Z

src/compiler/program.ts

+                        return oldProgram.structureIsReused = StructureIsReused.Not;
+                    }
+                }
+                else if (oldProgram.isSourceFileTargetOfRedirect.has(oldSourceFile.path)) {


Personally, I find it strange to call a set "is_".

amcasey

LGTM, but I don't know much about module resolution.

amcasey · 2017-07-13T21:30:43Z

src/compiler/types.ts

@@ -2330,12 +2330,12 @@ namespace ts {
         */
        /* @internal */ redirect?: {
            /** Source file this redirects to. */
-            readonly redirectTo: SourceFile,
+            readonly redirectTarget: SourceFile,
            /**
             * Source file for the duplicate package. This will not be used by the Program,
             * but we need to keep this around so we can watch for changes in underlying.


You might want to update the comments that still refer to "underlying".

amcasey · 2017-07-13T22:05:18Z

src/compiler/core.ts

-            }
-        }
-        return extensions;
+        return deduplicate([...allSupportedExtensions, ...extraFileExtensions.map(e => e.extension)]);


For my own edification, does this have some advantage over calling concat?

amcasey · 2017-07-13T22:20:38Z

src/compiler/moduleNameResolver.ts

+    /** Result of trying to resolve a module at a file. Needs to have 'packageId' added later. */
+    interface PathAndExtension {
+        path: string;
+        // (Use a different name than `extension` to make sure Resolved isn't assignable to PathAndExtension.)


Is it important that they not be assignable to each other? Is it just to avoid confusion or something more?

This prevents us from computing packageId and then not using it. See #12936

amcasey · 2017-07-13T22:23:49Z

src/compiler/moduleNameResolver.ts

@@ -891,13 +915,18 @@ namespace ts {
        // Even if extensions is DtsOnly, we can still look up a .ts file as a result of package.json "types"
        const nextExtensions = extensions === Extensions.DtsOnly ? Extensions.TypeScript : extensions;
        // Don't do package.json lookup recursively, because Node.js' package lookup doesn't.
-        return nodeLoadModuleByRelativeName(nextExtensions, file, failedLookupLocations, onlyRecordFailures, state, /*considerPackageJson*/ false);
+        const result = nodeLoadModuleByRelativeName(nextExtensions, file, failedLookupLocations, onlyRecordFailures, state, /*considerPackageJson*/ false);
+        if (result) {


What gets returned otherwise? Is return undefined implied?

Yes, JS functions always return undefined if they don't reach a return statement.

Personally, I like to be explicit, but I'm fine if this is idiomatic JS.

amcasey · 2017-07-13T22:26:25Z

src/compiler/program.ts

+                    // they might become redirects. So we must rebuild the program.
+                    const prevKind = seenPackageNames.get(packageName);
+                    const newKind = oldSourceFile === newSourceFile ? SeenPackageName.Exists : SeenPackageName.Modified;
+                    if (prevKind !== undefined && newKind === SeenPackageName.Modified || prevKind === SeenPackageName.Modified) {


Personally, I would parenthesize the && subexpression.

amcasey · 2017-07-13T22:32:06Z

src/compiler/utilities.ts

+            (oldResolution.packageId === newResolution.packageId || oldResolution.packageId && newResolution.packageId && packageIdIsEqual(oldResolution.packageId, newResolution.packageId));
+    }
+
+    function packageIdIsEqual(a: PackageId, b: PackageId): boolean {


Possibly this is following a precedent, but why not have packageIdIsEqual handle undefined?

amcasey · 2017-07-13T22:33:11Z

src/harness/harnessLanguageService.ts

-        getScriptFileNames(): string[] { return this.getFilenames(); }
+        getScriptFileNames(): string[] {
+            return this.getFilenames().filter(f =>
+                ts.isAnySupportedFileExtension(f) !== undefined);


Why check for undefined? Doesn't it return a boolean?

Good catch! We need --strictNullChecks...

amcasey · 2017-07-13T22:39:48Z

src/harness/unittests/reuseProgramStructure.ts

+
+            it("Target changes -> redirect broken", () => {
+                const program_1 = createRedirectProgram();
+                assert.lengthOf(program_1.getSemanticDiagnostics(), 0);


Would deepEquals provide more useful output in the event of a failure?

amcasey · 2017-07-13T22:40:46Z

src/harness/unittests/reuseProgramStructure.ts

+                assert.lengthOf(program_2.getSemanticDiagnostics(), 1);
+            });
+
+            it("Previously duplicate packages -> program structure not reused", () => {


I may have missed it, but is there a case where redirects are used and the structure is reused?

amcasey · 2017-07-13T22:41:18Z

src/harness/unittests/reuseProgramStructure.ts

    });

+    function updateProgramText(files: ReadonlyArray<NamedSourceText>, fileName: string, newProgramText: string) {
+        const file = find(files, f => f.name === fileName)!;


For my own edification, what does the ! mean?

Asserts (to the type-checker) that a value is not null or undefined. Has no effect as we don't have --strictNullChecks turned on, but I like to pretend. 😃

amcasey · 2017-07-13T22:43:04Z

What diagnostics, if any, would be reported if the duplicated module contained an error?

… was a redirect

ghost · 2017-07-14T17:30:37Z

See the test duplicatePackage_withErrors.

amcasey · 2017-07-14T18:29:01Z

Thanks for making those changes!

ghost · 2017-08-03T20:51:58Z

src/compiler/program.ts

+                let fileChanged: boolean;
+                if (oldSourceFile.redirect) {
+                    // We got `newSourceFile` by path, so it is actually for the unredirected file.
+                    // This lets us know if the unredirected file has changed. If it has we should break the redirect.


Move this comment inside of the inner if

aozgaa

LGTM, just a couple nits after our review yesterday.

aozgaa · 2017-07-14T22:47:49Z

src/compiler/core.ts


    export function getSupportedExtensions(options?: CompilerOptions, extraFileExtensions?: ReadonlyArray<JsFileExtensionInfo>): ReadonlyArray<string> {
        const needAllExtensions = options && options.allowJs;
        if (!extraFileExtensions || extraFileExtensions.length === 0 || !needAllExtensions) {
-            return needAllExtensions ? allSupportedExtensions : supportedTypeScriptExtensions;
+            // TODO: Return a ReadonlyArray<string> from this function to avoid casts. https://github.com/Microsoft/TypeScript/issues/16312


nit: "Return a ReadonlyArray<string> to avoid casts. see https://..."

aozgaa · 2017-07-19T19:37:15Z

src/harness/unittests/reuseProgramStructure.ts

@@ -109,7 +109,8 @@ namespace ts {
    function createTestCompilerHost(texts: NamedSourceText[], target: ScriptTarget, oldProgram?: ProgramWithSourceTexts): TestCompilerHost {
        const files = arrayToMap(texts, t => t.name, t => {
            if (oldProgram) {
-                const oldFile = <SourceFileWithText>oldProgram.getSourceFile(t.name);
+                let oldFile = <SourceFileWithText>oldProgram.getSourceFile(t.name);
+                if (oldFile && oldFile.redirect) oldFile = oldFile.redirect.unredirected;


Please use braces.

aozgaa · 2017-07-20T18:58:33Z

src/harness/unittests/reuseProgramStructure.ts

+
+                const program_2 = updateRedirectProgram(program_1, files => {
+                    updateProgramText(files, bxIndex, "export default class X { private x: number; private y: number; }");
+                    updateProgramText(files, bxPackage, JSON.stringify('{ name: "x", version: "1.2.4" }'));


Should you pass a string to JSON.stringify? This appears inconsistent across usages.

aozgaa · 2017-07-20T21:09:19Z

src/harness/unittests/reuseProgramStructure.ts

    });

+    function updateProgramText(files: ReadonlyArray<NamedSourceText>, fileName: string, newProgramText: string) {


Can you move this declaration next to updateProgram?

aozgaa · 2017-07-20T21:14:53Z

src/compiler/moduleNameResolver.ts

@@ -93,7 +105,8 @@ namespace ts {
        }
    }

-    function readJson(path: string, host: ModuleResolutionHost): { typings?: string, types?: string, main?: string } {
+    interface PackageJson { name?: string; version?: string; typings?: string; types?: string; main?: string; }


If this is just an implementation detail (as it appears to be for me), then move this declaration up with the others at the top of this file. Otherwise, move it to types.ts.

Please add newlines.

…the other (microsoft#16274) * For duplicate source files of the same package, make one redirect to the other * Add reuseProgramStructure tests * Copy `sourceFileToPackageId` and `isSourceFileTargetOfRedirect` only if we completely reuse old structure * Use fallthrough instead of early exit from loop * Use a set to efficiently detect duplicate package names * Move map setting outside of createRedirectSourceFile * Correctly handle seenPackageNames set * sourceFileToPackageId -> sourceFileToPackageName * Renames * Respond to PR comments * Fix bug where `oldSourceFile !== newSourceFile` because oldSourceFile was a redirect * Clean up redirectInfo * Respond to PR comments

msftclas added the cla-not-required label Jun 5, 2017

For duplicate source files of the same package, make one redirect to …

60b9aa3

…the other

ghost force-pushed the package-id branch from 211a2cf to 60b9aa3 Compare June 6, 2017 00:11

mhegazy suggested changes Jun 7, 2017

View reviewed changes

Andy Hanson added 8 commits June 7, 2017 13:28

Add reuseProgramStructure tests

8157308

Copy sourceFileToPackageId and isSourceFileTargetOfRedirect only …

e4d5f0d

…if we completely reuse old structure

Merge branch 'master' into package-id

17c5619

Use fallthrough instead of early exit from loop

1f32e62

Use a set to efficiently detect duplicate package names

f66713d

Move map setting outside of createRedirectSourceFile

04cae2e

Correctly handle seenPackageNames set

61b768a

sourceFileToPackageId -> sourceFileToPackageName

4d55f34

andy-ms added 2 commits June 7, 2017 15:51

Merge branch 'master' into package-id

d702180

Merge branch 'master' into package-id

e2a0931

ghost requested a review from amcasey July 10, 2017 21:07

ghost requested a review from aozgaa July 13, 2017 20:41

amcasey reviewed Jul 13, 2017

View reviewed changes

Renames

09e1c92

ghost force-pushed the package-id branch from 8bf33cb to 09e1c92 Compare July 13, 2017 20:57

Merge branch 'master' into package-id

d557e9a

amcasey approved these changes Jul 13, 2017

View reviewed changes

amcasey reviewed Jul 13, 2017

View reviewed changes

Respond to PR comments

3d2fc00

Fix bug where oldSourceFile !== newSourceFile because oldSourceFile…

018c9c5

… was a redirect

ghost commented Aug 3, 2017

View reviewed changes

Andy Hanson added 2 commits August 3, 2017 14:27

Clean up redirectInfo

91b2575

Merge branch 'master' into package-id

bec2d03

aozgaa approved these changes Aug 4, 2017

View reviewed changes

Andy Hanson added 2 commits August 9, 2017 13:44

Respond to PR comments

8408b3d

Merge branch 'master' into package-id

9f74f55

ghost merged commit 37b20ee into master Aug 9, 2017

ghost deleted the package-id branch August 9, 2017 21:39

intellix mentioned this pull request Aug 10, 2017

Angular CLI and monorepos (like lerna, yarn workspaces) angular/angular-cli#7097

Closed

devoto13 mentioned this pull request Jan 25, 2018

fix: host should not return a redirect source file from getSourceFile ng-packagr/ng-packagr#501

Closed

2 tasks

ghost mentioned this pull request Apr 10, 2018

Update outdated comments #23320

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

		});

		function updateProgramText(files: ReadonlyArray<NamedSourceText>, fileName: string, newProgramText: string) {

For duplicate source files of the same package, make one redirect to the other #16274

For duplicate source files of the same package, make one redirect to the other #16274

Conversation

ghost commented Jun 5, 2017 • edited by ghost Loading

ghost commented Jun 6, 2017 • edited by ghost Loading

mhegazy commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 7, 2017

amcasey commented Jul 11, 2017

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 14, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Jul 13, 2017

ghost commented Jul 14, 2017

amcasey commented Jul 14, 2017

Choose a reason for hiding this comment

aozgaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 5, 2017 •

edited by ghost

Loading

ghost commented Jun 6, 2017 •

edited by ghost

Loading

ghost Jul 14, 2017 •

edited by ghost

Loading