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

Cache accessibe symbol chains and serialized type parameter name generation #43973

Merged
merged 5 commits into from
May 12, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3935,12 +3935,12 @@ namespace ts {
return typeCopy;
}

function forEachSymbolTableInScope<T>(enclosingDeclaration: Node | undefined, callback: (symbolTable: SymbolTable) => T): T {
function forEachSymbolTableInScope<T>(enclosingDeclaration: Node | undefined, callback: (symbolTable: SymbolTable, ignoreQualification?: boolean, scopeNode?: Node) => T): T {
Copy link
Member

Choose a reason for hiding this comment

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

I dont see anywhere we are passing ignoreQualification true or false ?
Also can we make these parameter nonOptional but type | undefined instead to ensure every call back considers what needs to be passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Inside this function, it's not; however the function we often pass as a callback uses it as a second parameter (and it's optional there) for its' other callsites, so this optional parameter is here so the callback parameter is compatible with both that usage and the node-finding usage.

let result: T;
for (let location = enclosingDeclaration; location; location = location.parent) {
// Locals of a source file are not in scope (because they get merged into the global symbol table)
if (location.locals && !isGlobalSourceFile(location)) {
if (result = callback(location.locals)) {
if (result = callback(location.locals, /*ignoreQualification*/ undefined, location)) {
return result;
}
}
Expand All @@ -3955,7 +3955,7 @@ namespace ts {
// `sym` may not have exports if this module declaration is backed by the symbol for a `const` that's being rewritten
// into a namespace - in such cases, it's best to just let the namespace appear empty (the const members couldn't have referred
// to one another anyway)
if (result = callback(sym?.exports || emptySymbols)) {
if (result = callback(sym?.exports || emptySymbols, /*ignoreQualification*/ undefined, location)) {
return result;
}
break;
Expand All @@ -3976,7 +3976,7 @@ namespace ts {
(table || (table = createSymbolTable())).set(key, memberSymbol);
}
});
if (table && (result = callback(table))) {
if (table && (result = callback(table, /*ignoreQualification*/ undefined, location))) {
return result;
}
break;
Expand All @@ -3995,13 +3995,23 @@ namespace ts {
if (!(symbol && !isPropertyOrMethodDeclarationSymbol(symbol))) {
return undefined;
}
const links = getSymbolLinks(symbol);
const cache = (links.accessibleChainCache ||= new Map());
// Go from enclosingDeclaration to the first scope we check, so the cache is keyed off the scope and thus shared more
const firstRelevantLocation = forEachSymbolTableInScope(enclosingDeclaration, (_, __, node) => node);
const key = `${useOnlyExternalAliasing ? 0 : 1}|${firstRelevantLocation && getNodeId(firstRelevantLocation)}|${meaning}`;
if (cache.has(key)) {
weswigham marked this conversation as resolved.
Show resolved Hide resolved
return cache.get(key);
Copy link
Member Author

Choose a reason for hiding this comment

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

This cache could probably be better - intelligently sharing results for commonish-meanings and having a hierarchical cache both sound nice in theory, but are a pain to implement performantly (the last time I tried I ruined perf in the general case). This relatively simple cache was sufficient to bring the example runtime down from 20s to 7s on my machine, though is likely to only really effect really degenerate cases, like the one in the linked issue.

}

const id = getSymbolId(symbol);
let visitedSymbolTables = visitedSymbolTablesMap.get(id);
if (!visitedSymbolTables) {
visitedSymbolTablesMap.set(id, visitedSymbolTables = []);
}
return forEachSymbolTableInScope(enclosingDeclaration, getAccessibleSymbolChainFromSymbolTable);
const result = forEachSymbolTableInScope(enclosingDeclaration, getAccessibleSymbolChainFromSymbolTable);
cache.set(key, result);
return result;

/**
* @param {ignoreQualification} boolean Set when a symbol is being looked for through the exports of another symbol (meaning we have a route to qualify it already)
Expand Down Expand Up @@ -5814,7 +5824,7 @@ namespace ts {
}
if (context.flags & NodeBuilderFlags.GenerateNamesForShadowedTypeParams) {
const rawtext = result.escapedText as string;
let i = 0;
let i = context.typeParameterNamesByTextNextNameCount?.get(rawtext) || 0;
let text = rawtext;
while (context.typeParameterNamesByText?.has(text) || typeParameterShadowsNameInScope(text as __String, context, type)) {
i++;
Expand All @@ -5823,8 +5833,11 @@ namespace ts {
if (text !== rawtext) {
result = factory.createIdentifier(text, result.typeArguments);
}
(context.typeParameterNames || (context.typeParameterNames = new Map())).set(getTypeId(type), result);
(context.typeParameterNamesByText || (context.typeParameterNamesByText = new Set())).add(result.escapedText as string);
// avoiding iterations of the above loop turns out to be worth it when `i` starts to get large, so we cache the max
// `i` we've used thus far, to save work later
(context.typeParameterNamesByTextNextNameCount ||= new Map()).set(rawtext, i);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the "cache serialized type parameter name generation" part - this additional tiny bit of caching brought the runtime of the example down from 7s to 5.5s on my machine.

(context.typeParameterNames ||= new Map()).set(getTypeId(type), result);
(context.typeParameterNamesByText ||= new Set()).add(result.escapedText as string);
weswigham marked this conversation as resolved.
Show resolved Hide resolved
}
return result;
}
Expand Down Expand Up @@ -7737,6 +7750,7 @@ namespace ts {
typeParameterSymbolList?: Set<number>;
typeParameterNames?: ESMap<TypeId, Identifier>;
typeParameterNamesByText?: Set<string>;
typeParameterNamesByTextNextNameCount?: ESMap<string, number>;
usedSymbolNames?: Set<string>;
remappedSymbolNames?: ESMap<SymbolId, string>;
reverseMappedStack?: ReverseMappedSymbol[];
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4831,6 +4831,7 @@ namespace ts {
typeOnlyDeclaration?: TypeOnlyCompatibleAliasDeclaration | false; // First resolved alias declaration that makes the symbol only usable in type constructs
isConstructorDeclaredProperty?: boolean; // Property declared through 'this.x = ...' assignment in constructor
tupleLabelDeclaration?: NamedTupleMember | ParameterDeclaration; // Declaration associated with the tuple's label
accessibleChainCache?: ESMap<string, Symbol[] | undefined>;
}

/* @internal */
Expand Down
6 changes: 5 additions & 1 deletion src/harness/compilerImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,11 @@ namespace compiler {
if (compilerOptions.skipDefaultLibCheck === undefined) compilerOptions.skipDefaultLibCheck = true;
if (compilerOptions.noErrorTruncation === undefined) compilerOptions.noErrorTruncation = true;

const preProgram = ts.length(rootFiles) < 100 ? ts.createProgram(rootFiles || [], { ...compilerOptions, configFile: compilerOptions.configFile, traceResolution: false }, host) : undefined;
// pre-emit/post-emit error comparison requires declaration emit twice, which can be slow. If it's unlikely to flag any error consistency issues
// and if the test is running `skipLibCheck` - an indicator that we want the tets to run quickly - skip the before/after error comparison, too
const skipErrorComparison = ts.length(rootFiles) >= 100 || (!!compilerOptions.skipLibCheck && !!compilerOptions.declaration);
Copy link
Member Author

Choose a reason for hiding this comment

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

Declaration emit for this test still takes 5s on my machine. getPreEmitDiagnostics invokes declaration emit to get declaration emit errors (for historical reasons they've been considered "preEmit" errors), meaning declaration emit had to run twice, so 10s. Single-threaded, this was fine, but in high resource contention scenarios (ie, runtests-parallel) this was long enough to stretch to 40s and timeout. So I added an opt-out case to the preEmit/preEmit-after-emit error consistency check to keep this test runtime low (so it could pass without a timeout when running in parallel, and probably on CI).


const preProgram = !skipErrorComparison ? ts.createProgram(rootFiles || [], { ...compilerOptions, configFile: compilerOptions.configFile, traceResolution: false }, host) : undefined;
const preErrors = preProgram && ts.getPreEmitDiagnostics(preProgram);

const program = ts.createProgram(rootFiles || [], compilerOptions, host);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
tests/cases/compiler/Api.ts(6,5): error TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
tests/cases/compiler/Api.ts(7,5): error TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
tests/cases/compiler/Api.ts(8,5): error TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.


==== tests/cases/compiler/http-client.ts (0 errors) ====
type TPromise<ResolveType, RejectType = any> = Omit<Promise<ResolveType>, "then" | "catch"> & {
then<TResult1 = ResolveType, TResult2 = never>(
onfulfilled?: ((value: ResolveType) => TResult1 | PromiseLike<TResult1>) | undefined | null,
onrejected?: ((reason: RejectType) => TResult2 | PromiseLike<TResult2>) | undefined | null,
): TPromise<TResult1 | TResult2, RejectType>;
catch<TResult = never>(
onrejected?: ((reason: RejectType) => TResult | PromiseLike<TResult>) | undefined | null,
): TPromise<ResolveType | TResult, RejectType>;
};

export interface HttpResponse<D extends unknown, E extends unknown = unknown> extends Response {
data: D;
error: E;
}

export class HttpClient<SecurityDataType = unknown> {
public request = <T = any, E = any>(): TPromise<HttpResponse<T, E>> => {
return '' as any;
};
}
==== tests/cases/compiler/Api.ts (3 errors) ====
import { HttpClient } from "./http-client";

export class Api<SecurityDataType = unknown> {
constructor(private http: HttpClient<SecurityDataType>) { }

abc1 = () => this.http.request();
~~~~
!!! error TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
abc2 = () => this.http.request();
~~~~
!!! error TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
abc3 = () => this.http.request();
~~~~
!!! error TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
//// [tests/cases/compiler/declarationEmitPrivatePromiseLikeInterface.ts] ////

//// [http-client.ts]
type TPromise<ResolveType, RejectType = any> = Omit<Promise<ResolveType>, "then" | "catch"> & {
then<TResult1 = ResolveType, TResult2 = never>(
onfulfilled?: ((value: ResolveType) => TResult1 | PromiseLike<TResult1>) | undefined | null,
onrejected?: ((reason: RejectType) => TResult2 | PromiseLike<TResult2>) | undefined | null,
): TPromise<TResult1 | TResult2, RejectType>;
catch<TResult = never>(
onrejected?: ((reason: RejectType) => TResult | PromiseLike<TResult>) | undefined | null,
): TPromise<ResolveType | TResult, RejectType>;
};

export interface HttpResponse<D extends unknown, E extends unknown = unknown> extends Response {
data: D;
error: E;
}

export class HttpClient<SecurityDataType = unknown> {
public request = <T = any, E = any>(): TPromise<HttpResponse<T, E>> => {
return '' as any;
};
}
//// [Api.ts]
import { HttpClient } from "./http-client";

export class Api<SecurityDataType = unknown> {
constructor(private http: HttpClient<SecurityDataType>) { }

abc1 = () => this.http.request();
abc2 = () => this.http.request();
abc3 = () => this.http.request();
}

//// [http-client.js]
"use strict";
exports.__esModule = true;
exports.HttpClient = void 0;
var HttpClient = /** @class */ (function () {
function HttpClient() {
this.request = function () {
return '';
};
}
return HttpClient;
}());
exports.HttpClient = HttpClient;
//// [Api.js]
"use strict";
exports.__esModule = true;
exports.Api = void 0;
var Api = /** @class */ (function () {
function Api(http) {
var _this = this;
this.http = http;
this.abc1 = function () { return _this.http.request(); };
this.abc2 = function () { return _this.http.request(); };
this.abc3 = function () { return _this.http.request(); };
}
return Api;
}());
exports.Api = Api;


//// [http-client.d.ts]
declare type TPromise<ResolveType, RejectType = any> = Omit<Promise<ResolveType>, "then" | "catch"> & {
then<TResult1 = ResolveType, TResult2 = never>(onfulfilled?: ((value: ResolveType) => TResult1 | PromiseLike<TResult1>) | undefined | null, onrejected?: ((reason: RejectType) => TResult2 | PromiseLike<TResult2>) | undefined | null): TPromise<TResult1 | TResult2, RejectType>;
catch<TResult = never>(onrejected?: ((reason: RejectType) => TResult | PromiseLike<TResult>) | undefined | null): TPromise<ResolveType | TResult, RejectType>;
};
export interface HttpResponse<D extends unknown, E extends unknown = unknown> extends Response {
data: D;
error: E;
}
export declare class HttpClient<SecurityDataType = unknown> {
request: <T = any, E = any>() => TPromise<HttpResponse<T, E>, any>;
}
export {};
34 changes: 34 additions & 0 deletions tests/cases/compiler/declarationEmitPrivatePromiseLikeInterface.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// @declaration: true
// @skipLibCheck: true
// @noTypesAndSymbols: true
Copy link
Member Author

Choose a reason for hiding this comment

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

And I've skipped the type/symbol baselines for this test, since they double again the test runtime (and also thus cause a timeout) by essentially invoking the same logic as declaration emit for no real additional gain (compared to the declaration emit we're already running).

// @filename: http-client.ts
type TPromise<ResolveType, RejectType = any> = Omit<Promise<ResolveType>, "then" | "catch"> & {
then<TResult1 = ResolveType, TResult2 = never>(
onfulfilled?: ((value: ResolveType) => TResult1 | PromiseLike<TResult1>) | undefined | null,
onrejected?: ((reason: RejectType) => TResult2 | PromiseLike<TResult2>) | undefined | null,
): TPromise<TResult1 | TResult2, RejectType>;
catch<TResult = never>(
onrejected?: ((reason: RejectType) => TResult | PromiseLike<TResult>) | undefined | null,
): TPromise<ResolveType | TResult, RejectType>;
};

export interface HttpResponse<D extends unknown, E extends unknown = unknown> extends Response {
data: D;
error: E;
}

export class HttpClient<SecurityDataType = unknown> {
public request = <T = any, E = any>(): TPromise<HttpResponse<T, E>> => {
return '' as any;
};
}
// @filename: Api.ts
import { HttpClient } from "./http-client";

export class Api<SecurityDataType = unknown> {
constructor(private http: HttpClient<SecurityDataType>) { }

abc1 = () => this.http.request();
abc2 = () => this.http.request();
abc3 = () => this.http.request();
}