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.

Sign up for GitHub

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

Reduce polymorphism resulting from unstable Node shapes #51682

Merged
merged 11 commits into from
Dec 13, 2022
Merged

Reduce polymorphism resulting from unstable Node shapes #51682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ab3f38a
Move .symbol to Declaration
rbuckton Nov 11, 2022
3dce669
simplify some factories
rbuckton Nov 11, 2022
5a211fa
Move localSymbol to Declaration
rbuckton Nov 11, 2022
50e3eff
Ensure JSDocContainer types are properly initialized
rbuckton Nov 17, 2022
edc564a
Move contextualType from Node to NodeLinks
rbuckton Nov 17, 2022
4c94861
Move 'locals' and 'nextContainer' out of Node
rbuckton Nov 21, 2022
39b63a1
Move 'flowNode' out of 'Node'
rbuckton Nov 21, 2022
ecb2a64
Pre-define endFlowNode/returnFlowNode
rbuckton Nov 22, 2022
f5c1ca1
Pre-define some SourceFile properties and a more stable cloneNode
rbuckton Nov 29, 2022
d4c2ef3
Don't add excess properties to type nodes in typeToTypeNode
rbuckton Nov 29, 2022
afc0496
Refactor wrapSymbolTrackerToReportForContext to improve perf
rbuckton Dec 7, 2022
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,4 @@ tests/cases/user/puppeteer/puppeteer
tests/cases/user/axios-src/axios-src
tests/cases/user/prettier/prettier
.eslintcache
*v8.log
Copy link
Contributor

Choose a reason for hiding this comment

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

a question, if I may, what kind of tools you were using to investigate those things? I imagine that in the case of Nodes the issue was already known for some time but I wonder how I could test those things in my libraries.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have an internal tool I wrote to analyze log files generated by various V8 commandline options.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Andarist One open source tool that might be of interest is https://github.com/thlorenz/deoptigate

258 changes: 138 additions & 120 deletions src/compiler/binder.ts
View file
Open in desktop

Large diffs are not rendered by default.

852 changes: 489 additions & 363 deletions src/compiler/checker.ts
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 8 additions & 6 deletions src/compiler/emitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
BundleFileTextLikeKind,
CallExpression,
CallSignatureDeclaration,
canHaveLocals,
CaseBlock,
CaseClause,
CaseOrDefaultClause,
Expand Down Expand Up @@ -181,6 +182,7 @@ import {
getTransformers,
getTypeNode,
guessIndentation,
HasLocals,
hasRecordedExternalHelpers,
HeritageClause,
Identifier,
Expand Down Expand Up @@ -412,6 +414,7 @@ import {
tracing,
TransformationResult,
transformNodes,
tryCast,
tryParseRawSourceMap,
TryStatement,
TupleTypeNode,
Expand Down Expand Up @@ -2475,8 +2478,7 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
//

function emitPrivateIdentifier(node: PrivateIdentifier) {
const writeText = node.symbol ? writeSymbol : write;
writeText(getTextOfNode(node, /*includeTrivia*/ false), node.symbol);
write(getTextOfNode(node, /*includeTrivia*/ false));
}


Expand Down Expand Up @@ -5656,9 +5658,9 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
/**
* Returns a value indicating whether a name is unique within a container.
*/
function isUniqueLocalName(name: string, container: Node): boolean {
for (let node = container; isNodeDescendantOf(node, container); node = node.nextContainer!) {
if (node.locals) {
function isUniqueLocalName(name: string, container: HasLocals | undefined): boolean {
for (let node = container; node && isNodeDescendantOf(node, container); node = node.nextContainer) {
if (canHaveLocals(node) && node.locals) {
const local = node.locals.get(escapeLeadingUnderscores(name));
// We conservatively include alias symbols to cover cases where they're emitted as locals
if (local && local.flags & (SymbolFlags.Value | SymbolFlags.ExportValue | SymbolFlags.Alias)) {
Expand Down Expand Up @@ -5798,7 +5800,7 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
function generateNameForModuleOrEnum(node: ModuleDeclaration | EnumDeclaration) {
const name = getTextOfNode(node.name);
// Use module/enum name itself if it is unique, otherwise make a unique variation
return isUniqueLocalName(name, node) ? name : makeUniqueName(name, isUniqueName, /*optimistic*/ false, /*scoped*/ false, /*privateName*/ false, /*prefix*/ "", /*suffix*/ "");
return isUniqueLocalName(name, tryCast(node, canHaveLocals)) ? name : makeUniqueName(name, isUniqueName, /*optimistic*/ false, /*scoped*/ false, /*privateName*/ false, /*prefix*/ "", /*suffix*/ "");
}

/**
Expand Down
Loading