Skip to content

Improve performance of Debug.format functions #49487

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

Merged
merged 3 commits into from
Jun 10, 2022
Merged

Improve performance of Debug.format functions #49487

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ea3fb3
Cache enum members for Debug.format functions
jakebailey Jun 10, 2022
c5b48ba
Use an array join rather than temporary strings in formatEnum
jakebailey Jun 10, 2022
fca4519
Formatting
jakebailey Jun 10, 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
20 changes: 16 additions & 4 deletions src/compiler/debug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,19 +301,19 @@ namespace ts {
return members.length > 0 && members[0][0] === 0 ? members[0][1] : "0";
}
if (isFlags) {
let result = "";
const result: string[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's still the case, but in earlier v8 versions if you avoid allocating the array until you have the first element the performance is improved because it doesn't have to re-type the array.

const ar1 = []; // typed internally as a JS_ARRAY_TYPE of PACKED_SMI_ELEMENTS
ar1.push("foo"); // deoptimizes, re-types, now it's internally typed as an array of strings

// vs

const ar2 = ["foo"]; // typed internally as an array of strings

As a result, we have a lot of code that does either of the following:

let array: Foo[] | undefined;
...
if (array) {
  array.push(foo);
}
else {
  array = [foo];
}

// or
...
array = append(array, foo); // same as above, but in an inlinable function

That said, since this code isn't performance critical, it probably wouldn't help much.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an interesting idea; I know that this string addition itself was definitely bad, but I had never (when fixing this kind of code after pprof showing huge allocations) thought about creating a single array with one element to get v8 to do the right thing; the performance boost from just doing join was the main significant item.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, this was a performance optimization employed by @vladima back in the day. It still seems to be effective from what I can tell.

let remainingFlags = value;
for (const [enumValue, enumName] of members) {
if (enumValue > value) {
break;
}
if (enumValue !== 0 && enumValue & value) {
result = `${result}${result ? "|" : ""}${enumName}`;
result.push(enumName);
remainingFlags &= ~enumValue;
}
}
if (remainingFlags === 0) {
return result;
return result.join("|");
}
}
else {
Expand All @@ -326,7 +326,17 @@ namespace ts {
return value.toString();
}

const enumMemberCache = new Map<any, SortedReadonlyArray<[number, string]>>();

function getEnumMembers(enumObject: any) {
// Assuming enum objects do not change at runtime, we can cache the enum members list
// to reuse later. This saves us from reconstructing this each and every time we call
// a formatting function (which can be expensive for large enums like SyntaxKind).
const existing = enumMemberCache.get(enumObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

As you mentioned, this is one of those things that only really matters for tests. In tsc there will never be a cached value since these functions should only ever be called right before a crash due to a debug assertion. It can possibly benefit the language service, but I'm not sure how often that would happen in practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main benefit at runtime is the debug helpers that we define to use in VS Code's debugger in settings.json, where these helpers are called over and over to show the node types. Some things can be improved later, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

is there any document describing how you debug TypeScript etc? I have my own ways (mostly using __debugTypeToString and __debugGetText in the debugger) - but I wonder if I'm missing some better techniques.

Copy link
Member Author

Choose a reason for hiding this comment

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

Making sure that customDescriptionGenerator is enabled in your launch.json is one thing related to this PR, but past that, I don't have any cool tricks besides a collection of helpers I used in the watch window (https://github.com/jakebailey/ts-debug-helpers), which is mainly about printing out nodes/the parser's scanner to more easily see what code is being operated on. Not type related at all.

if (existing) {
return existing;
}

const result: [number, string][] = [];
for (const name in enumObject) {
const value = enumObject[name];
Expand All @@ -335,7 +345,9 @@ namespace ts {
}
}

return stableSort<[number, string]>(result, (x, y) => compareValues(x[0], y[0]));
const sorted = stableSort<[number, string]>(result, (x, y) => compareValues(x[0], y[0]));
enumMemberCache.set(enumObject, sorted);
return sorted;
}

export function formatSyntaxKind(kind: SyntaxKind | undefined): string {
Expand Down