-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
[wip] Remove objectAllocator
concept
#51913
base: main
Are you sure you want to change the base?
Conversation
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at 1b0a6bf. You can monitor the build here. Update: The results are in! |
1b0a6bf
to
1df98f9
Compare
@rbuckton Here they are:
CompilerComparison Report - main..51913
System
Hosts
Scenarios
TSServerComparison Report - main..51913
System
Hosts
Scenarios
StartupComparison Report - main..51913
System
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at 8cfbe01. You can monitor the build here. Update: The results are in! |
8cfbe01
to
dc39df6
Compare
@rbuckton Here they are:
CompilerComparison Report - main..51913
System
Hosts
Scenarios
TSServerComparison Report - main..51913
System
Hosts
Scenarios
StartupComparison Report - main..51913
System
Hosts
Scenarios
Developer Information: |
That's quite good. Barely a change to the tsc benchmarks, but some nice improvements in the lsp benchmarks. I have a few other changes to try before taking this out of draft. |
dc39df6
to
3b862b6
Compare
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at 95b6afd. You can monitor the build here. Update: The results are in! |
@rbuckton Here they are:
CompilerComparison Report - main..51913
System
Hosts
Scenarios
TSServerComparison Report - main..51913
System
Hosts
Scenarios
StartupComparison Report - main..51913
System
Hosts
Scenarios
Developer Information: |
95b6afd
to
c2c2643
Compare
c2c2643
to
c6ffac9
Compare
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at c6ffac9. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - main..51913
System
Hosts
Scenarios
TSServerComparison Report - main..51913
System
Hosts
Scenarios
StartupComparison Report - main..51913
System
Hosts
Scenarios
Developer Information: |
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
e539966
to
9f19e4c
Compare
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@@ -503,15 +522,16 @@ export namespace Debug { | |||
return formatEnum(facts, (ts as any).TypeFacts, /*isFlags*/ true); | |||
} | |||
|
|||
const debugDescriptionSymbol = Symbol.for("debug.description"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super cool; I didn't realize this got added!
this.checker = checker; | ||
} | ||
|
||
getFlags(): TypeFlags { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if we need to add some sort of lint rule for ourselves to prevent us from needlessly using the methods in the compiler, but I'm not sure how efficient that would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a mechanism to handle that in an earlier commit. I could bring it back if we think it's necessary:
TypeScript/src/compiler/types.ts
Lines 9827 to 9851 in cadf6e8
/** | |
* Resolves to a type only when the 'services' project is loaded. Otherwise, results in `never`. | |
* @internal | |
*/ | |
export type ServicesOnlyType<T, Fallback = never> = ServicesForwardRefs extends { __services: true } ? T : Fallback; | |
/** | |
* Resolves a forward-reference to a type declared in the 'services' project. | |
* If 'services' is not present, results in `never`. | |
* @internal | |
*/ | |
export type ServicesForwardRef<K extends string> = ServicesForwardRefs extends { [P in K]: infer T } ? T : never; | |
/** | |
* Resolves a forward-reference to an array of a type declared in the 'services' project. | |
* If 'services' is not present, results in `never`. | |
* @internal | |
*/ | |
export type ServicesForwardRefArray<K extends string> = ServicesOnlyType<ServicesForwardRef<K>[]>; | |
/** | |
* A registry of forward references declared in the 'services' project. | |
* @internal | |
*/ | |
export interface ServicesForwardRefs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it might be worth it just to prevent accidental usage, since these are defined on the actual Type
interface too. But it's a little goofy to complain when they are now definitely actually declared in the compiler project? (Some stuff is shimmed out and throws, though...)
export function addObjectAllocatorPatcher(fn: (objectAllocator: ObjectAllocator) => void) { | ||
objectAllocatorPatchers.push(fn); | ||
fn(objectAllocator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What mechanism are we going to use in the future to tack stuff onto our objects for deprecations? Just throwing things onto the prototype? (That certainly sounds cleaner, though I just removed the one example...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patching the prototype seems to cause memory usage to skyrocket. I'd maybe do what I'm doing with SymbolObjectInternals
, such that using the method throws in TSC, but the deprecation code could patch that object with a version that works (with warnings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense; just didn't want to accidentally close the door and I did remove the one test we had for that perf which made it harder for you. 🙁
@@ -487,7 +494,8 @@ export function addNodeFactoryPatcher(fn: (factory: NodeFactory) => void) { | |||
* | |||
* @internal | |||
*/ | |||
export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNodeFactory): NodeFactory { | |||
export function createNodeFactory(flags: NodeFactoryFlags): NodeFactory { | |||
const markSynthetic = (flags & NodeFactoryFlags.MarkSynthetic) === NodeFactoryFlags.MarkSynthetic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I still have a bit more investigating to do into the memory overhead in self-build-src-public-api, but this is close enough to turn it into an actual PR. |
The title is still |
@@ -78,10 +78,11 @@ import { | |||
SnippetKind, | |||
SortedReadonlyArray, | |||
stableSort, | |||
Symbol, | |||
type Symbol, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest flipping this change back (since it's not needed), but I guess the cat's out of the bag anyway now that we correctly organize imports (or just use dprint to do it quickly), and direct imports are probably fine too 😄
this.checker = checker; | ||
} | ||
|
||
getFlags(): TypeFlags { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it might be worth it just to prevent accidental usage, since these are defined on the actual Type
interface too. But it's a little goofy to complain when they are now definitely actually declared in the compiler project? (Some stuff is shimmed out and throws, though...)
/** @internal */ original?: Node | undefined; // The original node if this is an updated node. | ||
/** @internal */ emitNode?: EmitNode | undefined; // Associated EmitNode (initialized by transforms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be like original: Node | undefined
? We don't use exactOptionalPropertyTypes
(yet?), so this change is a noop.
@@ -5846,6 +5909,45 @@ export const enum SymbolFlags { | |||
LateBindingContainer = Class | Interface | TypeLiteral | ObjectLiteral | Function, | |||
} | |||
|
|||
export interface SymbolDisplayPart { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of a bummer to have to have these here, but I don't see a way around that. Why oh why did we introduce the methods... 😄
export function addObjectAllocatorPatcher(fn: (objectAllocator: ObjectAllocator) => void) { | ||
objectAllocatorPatchers.push(fn); | ||
fn(objectAllocator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense; just didn't want to accidentally close the door and I did remove the one test we had for that perf which made it harder for you. 🙁
/** @internal */ | ||
export function formatStringFromArgs(text: string, args: DiagnosticArguments): string { | ||
return text.replace(/{(\d+)}/g, (_match, index: string) => "" + Debug.checkDefined(args[+index])); | ||
export function formatStringFromArgs(text: string, args: ArrayLike<string | number>, baseIndex = 0): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What required this to change signature? (I can't find it in the diff.)
import { | ||
SymbolObject, | ||
} from "../compiler/objectConstructors"; | ||
import { SignatureObjectInternals } from "../compiler/signatureObjectInternals"; | ||
import { SymbolObjectInternals } from "../compiler/symbolObjectInternals"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just do this via the namespaces? Half worried about the output ordering here but clearly it works so, it's probably fine. My other PR to sort imports more strictly is now moot because we dropped the plugin, so I can't enforce it anyway.
No description provided.