[Experiment] Direct imports

[a-tarasyuk]

@jakebailey #51443 (comment) Curious about how direct imports impact performance

[typescript-bot]

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

[jakebailey]

Impressive; this one is definitely tricky as there are still cycles. There is tooling to help with that.

FWIW you can just start using Object.entries unconditionally; the new target is ES2018 so a lot more is fair game, though I haven't made an issue for that and we will need to performance test as we suspect that our hand written ones are faster.

[a-tarasyuk]

this one is definitely tricky as there are still cycles.

Yep :(. I decided to start by converting to direct imports, the second step should be to resolve the circular dependencies.

There is tooling to help with that.

I know about madge, maybe there is a better tool...

[jakebailey]

Madge is the one I had tried, but I forgot the name and while searching I found https://github.com/Soontao/cycle-import-check

Funnily, the most reliable one I found was running rollup on the codebase, since it complains about it too.

Maybe using type imports would help the tooling as well.

[jakebailey]

But seriously, you got further than I did. I'm really shocked you got the tests to pass and tsc to run. I gave up and started working on other stuff instead...

[a-tarasyuk]

Debug has several circular uses

core.ts <-> debug.ts
debug.ts <-> version.ts

Can we move asserts to a new assert.ts module?

TypeScript/src/compiler/debug.ts

Lines 135 to 142 in b553aff

	export function fail(message?: string, stackCrawlMark?: AnyFunction): never {
	debugger;
	const e = new Error(message ? `Debug Failure. ${message}` : "Debug Failure.");
	if ((Error as any).captureStackTrace) {
	(Error as any).captureStackTrace(e, stackCrawlMark || fail);
	}
	throw e;
	}

TypeScript/src/compiler/debug.ts

Lines 150 to 208 in b553aff

	export function assert(expression: unknown, message?: string, verboseDebugInfo?: string | (() => string), stackCrawlMark?: AnyFunction): asserts expression {
	if (!expression) {
	message = message ? `False expression: ${message}` : "False expression.";
	if (verboseDebugInfo) {
	message += "\r\nVerbose Debug Information: " + (typeof verboseDebugInfo === "string" ? verboseDebugInfo : verboseDebugInfo());
	}
	fail(message, stackCrawlMark || assert);
	}
	}
	export function assertEqual<T>(a: T, b: T, msg?: string, msg2?: string, stackCrawlMark?: AnyFunction): void {
	if (a !== b) {
	const message = msg ? msg2 ? `${msg} ${msg2}` : msg : "";
	fail(`Expected ${a} === ${b}. ${message}`, stackCrawlMark || assertEqual);
	}
	}
	export function assertLessThan(a: number, b: number, msg?: string, stackCrawlMark?: AnyFunction): void {
	if (a >= b) {
	fail(`Expected ${a} < ${b}. ${msg || ""}`, stackCrawlMark || assertLessThan);
	}
	}
	export function assertLessThanOrEqual(a: number, b: number, stackCrawlMark?: AnyFunction): void {
	if (a > b) {
	fail(`Expected ${a} <= ${b}`, stackCrawlMark || assertLessThanOrEqual);
	}
	}
	export function assertGreaterThanOrEqual(a: number, b: number, stackCrawlMark?: AnyFunction): void {
	if (a < b) {
	fail(`Expected ${a} >= ${b}`, stackCrawlMark || assertGreaterThanOrEqual);
	}
	}
	export function assertIsDefined<T>(value: T, message?: string, stackCrawlMark?: AnyFunction): asserts value is NonNullable<T> {
	// eslint-disable-next-line no-null/no-null
	if (value === undefined || value === null) {
	fail(message, stackCrawlMark || assertIsDefined);
	}
	}
	export function checkDefined<T>(value: T | null | undefined, message?: string, stackCrawlMark?: AnyFunction): T {
	assertIsDefined(value, message, stackCrawlMark || checkDefined);
	return value;
	}
	export function assertEachIsDefined<T extends Node>(value: NodeArray<T>, message?: string, stackCrawlMark?: AnyFunction): asserts value is NodeArray<T>;
	export function assertEachIsDefined<T>(value: readonly T[], message?: string, stackCrawlMark?: AnyFunction): asserts value is readonly NonNullable<T>[];
	export function assertEachIsDefined<T>(value: readonly T[], message?: string, stackCrawlMark?: AnyFunction) {
	for (const v of value) {
	assertIsDefined(v, message, stackCrawlMark || assertEachIsDefined);
	}
	}
	export function checkEachDefined<T, A extends readonly T[]>(value: A, message?: string, stackCrawlMark?: AnyFunction): A {
	assertEachIsDefined(value, message, stackCrawlMark || checkEachDefined);
	return value;
	}

[jakebailey]

My Debug PR (#51455) does mend some of this, though it's not final at all.

I'm not really sure how far we want to stretch debug; I think that for now the best we should try to do is to make debug come first and then ensure it doesn't call anything. My other PR pulls deprecation code out into the deprecation compat file, which I think is the main blocker and I'll just go pull that into its own PR instead.

[DanielRosenwasser]

@typescript-bot perf test this faster

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at 9f7d90d. You can monitor the build here.

Update: The results are in!

[typescript-bot]

@DanielRosenwasser
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..51504

Metric	main	51504	Delta	Best	Worst
Angular - node (v16.17.1, x64)
Memory used	340,522k (± 0.02%)	340,520k (± 0.02%)	-2k (- 0.00%)	340,256k	340,603k
Parse Time	1.88s (± 0.66%)	1.89s (± 0.53%)	+0.00s (+ 0.16%)	1.87s	1.91s
Bind Time	0.65s (± 0.89%)	0.65s (± 0.69%)	-0.00s (- 0.77%)	0.64s	0.66s
Check Time	5.16s (± 0.38%)	5.17s (± 0.47%)	+0.00s (+ 0.10%)	5.12s	5.22s
Emit Time	5.11s (± 0.76%)	5.09s (± 1.16%)	-0.02s (- 0.39%)	4.96s	5.21s
Total Time	12.81s (± 0.46%)	12.79s (± 0.53%)	-0.01s (- 0.11%)	12.65s	12.94s
Compiler-Unions - node (v16.17.1, x64)
Memory used	188,257k (± 0.65%)	188,201k (± 0.64%)	-56k (- 0.03%)	186,475k	189,899k
Parse Time	0.79s (± 0.75%)	0.79s (± 0.96%)	-0.01s (- 1.13%)	0.77s	0.80s
Bind Time	0.42s (± 0.53%)	0.41s (± 0.82%)	-0.01s (- 1.90%)	0.41s	0.42s
Check Time	6.05s (± 0.72%)	6.01s (± 0.72%)	-0.04s (- 0.66%)	5.91s	6.11s
Emit Time	1.91s (± 0.72%)	1.88s (± 0.91%)	-0.02s (- 1.31%)	1.85s	1.93s
Total Time	9.17s (± 0.47%)	9.09s (± 0.64%)	-0.08s (- 0.87%)	8.98s	9.21s
Monaco - node (v16.17.1, x64)
Memory used	319,866k (± 0.01%)	319,915k (± 0.06%)	+49k (+ 0.02%)	319,757k	320,698k
Parse Time	1.43s (± 0.73%)	1.41s (± 0.67%)	-0.01s (- 0.91%)	1.39s	1.43s
Bind Time	0.59s (± 1.00%)	0.59s (± 0.57%)	-0.00s (- 0.17%)	0.59s	0.60s
Check Time	4.87s (± 0.39%)	4.87s (± 0.56%)	-0.01s (- 0.14%)	4.79s	4.92s
Emit Time	2.72s (± 0.90%)	2.72s (± 0.74%)	+0.00s (+ 0.07%)	2.68s	2.78s
Total Time	9.61s (± 0.40%)	9.59s (± 0.44%)	-0.02s (- 0.22%)	9.48s	9.71s
TFS - node (v16.17.1, x64)
Memory used	282,300k (± 0.01%)	282,234k (± 0.01%)	-67k (- 0.02%)	282,176k	282,310k
Parse Time	1.17s (± 0.51%)	1.17s (± 1.09%)	+0.00s (+ 0.17%)	1.14s	1.20s
Bind Time	0.65s (± 3.63%)	0.66s (± 3.50%)	+0.01s (+ 1.38%)	0.60s	0.70s
Check Time	4.76s (± 0.39%)	4.79s (± 0.32%)	+0.03s (+ 0.57%)	4.74s	4.82s
Emit Time	2.76s (± 2.01%)	2.75s (± 1.65%)	-0.02s (- 0.58%)	2.66s	2.84s
Total Time	9.35s (± 0.72%)	9.37s (± 0.67%)	+0.02s (+ 0.18%)	9.23s	9.50s
material-ui - node (v16.17.1, x64)
Memory used	435,297k (± 0.01%)	435,257k (± 0.00%)	-40k (- 0.01%)	435,233k	435,295k
Parse Time	1.65s (± 0.50%)	1.64s (± 0.57%)	-0.02s (- 0.97%)	1.62s	1.66s
Bind Time	0.50s (± 0.98%)	0.50s (± 0.80%)	-0.00s (- 0.99%)	0.49s	0.51s
Check Time	11.96s (± 0.42%)	11.77s (± 0.90%)	-0.19s (- 1.61%)	11.58s	11.99s
Emit Time	0.00s (± 0.00%)	0.00s (± 0.00%)	0.00s ( NaN%)	0.00s	0.00s
Total Time	14.13s (± 0.36%)	13.91s (± 0.80%)	-0.21s (- 1.52%)	13.69s	14.15s
xstate - node (v16.17.1, x64)
Memory used	516,263k (± 0.01%)	516,257k (± 0.01%)	-7k (- 0.00%)	516,142k	516,373k
Parse Time	2.33s (± 0.45%)	2.31s (± 0.24%)	-0.02s (- 0.77%)	2.30s	2.32s
Bind Time	0.84s (± 1.90%)	0.83s (± 3.11%)	-0.01s (- 1.07%)	0.78s	0.91s
Check Time	1.36s (± 0.50%)	1.36s (± 0.81%)	+0.01s (+ 0.52%)	1.33s	1.38s
Emit Time	0.06s (± 0.00%)	0.06s (± 0.00%)	0.00s ( 0.00%)	0.06s	0.06s
Total Time	4.60s (± 0.44%)	4.58s (± 0.57%)	-0.02s (- 0.44%)	4.52s	4.63s

System

Machine Name	ts-ci-ubuntu
Platform	linux 5.4.0-131-generic
Architecture	x64
Available Memory	16 GB
Available Memory	15 GB
CPUs	4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz

Hosts

• node (v16.17.1, x64)

Scenarios

• Angular - node (v16.17.1, x64)
• Compiler-Unions - node (v16.17.1, x64)
• Monaco - node (v16.17.1, x64)
• TFS - node (v16.17.1, x64)
• material-ui - node (v16.17.1, x64)
• xstate - node (v16.17.1, x64)

Benchmark	Name	Iterations
Current	51504	10
Baseline	main	10

Developer Information:

Download Benchmark

[jakebailey]

If this is to be trusted, then it's a tiny win. I was expecting roughly no change since esbuild devirtualizes everything already in the current bundle. But maybe there's more being done and we can diff the two tsc.js files.

[a-tarasyuk]

then it's a tiny win

Yep, -1% is not a big win. That's what I wanted to check. Maybe using _namespaces is cleaner and without significant performance loss.

[jakebailey]

My general feeling (and I think this is shared) is that the namespace stuff is really just a temporary hack and we'd like to have direct imports anyhow.

[jakebailey]

Are you planning on reopening this again somewhere else? I am wanting this change, though I don't know when we will "click the button" so I understand that it'd be a maintenance pain until then.

[a-tarasyuk]

Honestly, I just wanted to check the performance impact. These changes are difficult to maintain and after merging this PR #51565, these changes will be completely useless as they will affect all imports in all files. I think it would take less time to make all the changes from scratch than to fix merge conflicts :).

[jakebailey]

No problem! I appreciate that you showed it was possible, at least.

[a-tarasyuk]

@jakebailey Write me when you decide to do this migration and I'll try to restore these changes.