Skip to content

Make new harness fake host more performant in large complications #23951

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

Merged
merged 2 commits into from
May 8, 2018

Conversation

weswigham
Copy link
Member

This fixes some of the issues our RWC suite has with the new vfs-backed test harness.
The changes include:
First: empty path segments will now be collapsed by our path handling logic (as they were before). We had tests exhibiting a dependence on paths like ../..//site/SomeFile.ts resolving correctly.

Second: Do not attempt to regex away comments from .json files inside vfs.System. Instead, just use parseConfigFileTextToJson in the moduleNameResolver to allow for comments in parsed package.jsons (after all, we handle them in every other json document we parse, so why not there) - the regex used for finding comments and stripping them at the system layer had a catastrophic bad case in the RWC suite; our own parser should be more predictable (and more importantly, if it exhibits similar problems in the future, must be fixed).

Third: Replaces outputs in the fakes.CompilerHost with a canonicalized map and ordered list of names - linerarly searching for the insertion index in the output list took forever when there was > 2000 files in the output (as shown by our largest RWC tests) - this remedies that pathological case.

I think the VFS is still the root cause of some other RWC failures related to being unable to find a tsconfig, but I haven't gotten a chance to look into that yet, so that'll probably be separate.

@weswigham weswigham requested review from sandersn and rbuckton May 8, 2018 00:02
Copy link
Contributor

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

I don't think we should modify outputs as it's a verbatim list. Perhaps you could look into using the CompilerResult object in the RWC tests?

@@ -202,7 +200,8 @@ namespace fakes {
export class CompilerHost implements ts.CompilerHost {
public readonly sys: System;
public readonly defaultLibLocation: string;
public readonly outputs: documents.TextDocument[] = [];
private readonly outputNames: string[] = [];
private readonly outputsMap: ts.Map<documents.TextDocument> = ts.createMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

CompilerResult in harness/compiler.ts is much better at this than outputs

else {
this.outputs[index] = document;
const canonical = ts.toPath(fileName, this.sys.getCurrentDirectory(), ts.createGetCanonicalFileName(this.sys.useCaseSensitiveFileNames));
if (!this.outputsMap.has(canonical)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The canonicalization we do is very aggressive, which is one of the reasons CompilerResult uses a SortedMap. Also, this would completely mask the actual paths we use when we emit files.

Copy link
Member Author

@weswigham weswigham May 8, 2018

Choose a reason for hiding this comment

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

Not true - the path is only used as a cache key, the document itself still has its originally assigned path.

Copy link
Contributor

Choose a reason for hiding this comment

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

My apologies, as you are correct on my second point. What I meant to say was that I was thinking of something more like this (which wouldn't regenerate output on every dot-property access):

export class CompilerHost ... {
  ...
  private _outputs: documents.TextDocument[] = [];
  private _outputsIndices: collections.SortedMap<string, number>;

  constructor(...) {
    ...
    this._outputsIndices = new collections.SortedMap<string, number>(sys.vfs.stringComparer);
  }

  public get outputs(): ReadonlyArray<documents.TextDocument> {
    return this._outputs;
  }

  public writeFile(...) {
    ...
    let index = this._outputsIndices.get(document.file);
    if (index === undefined) {
        index = this._outputs.length;
        this._outputs.push(document);
        this._outputsIndices.set(document.file, index);
    }
    else {
        this._outputs[index] = document;
    }    
  }

Note that sys.vfs.stringComparer actually performs path comparisons based on the case sensitivity of the vfs. As a result, it compares things like C:\A\B\..\C and c:/a/c as equal on a case-insensitive vfs.

@weswigham
Copy link
Member Author

I don't think we should modify outputs as it's a verbatim list.

Modifying it is precisely what we were doing before. (And was why it was slow) Now its generated on demand and isn't searched through for insertion positions.

@rbuckton
Copy link
Contributor

rbuckton commented May 8, 2018

Modifying it is precisely what we were doing before.

You are correct, sorry. That's what I get for trying review a PR over my phone.

@weswigham
Copy link
Member Author

@rbuckton is this acceptable now?

@weswigham weswigham merged commit 556c316 into microsoft:master May 8, 2018
@weswigham weswigham deleted the fix-vfs-for-rwc branch May 8, 2018 19:46
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants