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

Detect slow loading of barrel modules #17

Closed
wants to merge 2 commits into from

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Jan 25, 2022

We've seen some projects slowed down by pulling in a huge number of files from a library when they really only wanted a small portion of the functionality. Ideally, the compiler itself would recognize this but, until then, flagging them could help people make spot fixes. Unfortunately, this seems to have a lot of false positive, esp in monorepos where node_modules might actually be user code.

Part of #2.

@amcasey
Copy link
Member Author

amcasey commented Jan 25, 2022

I'm dissatisfied with the output this produces for real traces.

@amcasey amcasey closed this Jan 25, 2022
@@ -32,6 +32,7 @@ const thresholdDuration = argv.forceMillis * 1000; // microseconds
const minDuration = argv.skipMillis * 1000; // microseconds
const minPercentage = 0.6;
const importExpressionThreshold = 10;
const sourceFileFanoutThreshold = 5;
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 was for local testing - the real value would be 50+.

@@ -52,6 +53,7 @@ interface EventSpan {
start: number;
end: number;
children: EventSpan[];
droppedChildCount: number;
Copy link
Member Author

Choose a reason for hiding this comment

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

Events under minDuration are dropped entirely, making it hard to approximate how many children a findSourceFiles event actually has. This fix was incomplete because the compiler uses sampling to drop short events from the trace before we even get it.

@@ -157,10 +160,13 @@ async function main(): Promise<void> {

const parent = stack[i];
const duration = span.end - span.start;
if (duration >= thresholdDuration || duration >= minPercentage * (parent.end - parent.start)) {
if (duration >= minDuration && (duration >= thresholdDuration || (span.event?.cat !== "program" && duration >= minPercentage * (parent.end - parent.start)))) {
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 was trying to exclude findSourceFile events from being included simply because they were large percentages of their parents. The parent-child relationship is less important for findSourceFile since eliminating the parent just moves the child to the next event that requires it (think about trying to eliminate a @types/node dependency by cleaning up the first file that pulls it in).

case "findSourceFile":
const sourcePath = event.args!.fileName;
const childCount = curr.children.length + curr.droppedChildCount;
if (childCount < sourceFileFanoutThreshold || !/node_modules/.test(sourcePath) || /@types\/node/.test(sourcePath)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

In user code, it's definitely uninteresting. @types/node seemed special enough to get an exception too, but it turns out that there are many exceptions - more than true positives, probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant