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

Tasks flamegraph generation #3048

Merged
merged 12 commits into from
Aug 27, 2022
Merged

Conversation

alcuadrado
Copy link
Member

@alcuadrado alcuadrado commented Aug 17, 2022

First PR of the compilation times optimization effort.

This PR introduces a Hardhat argument --flamegraph, which creates a flamegraph of the Hardhat tasks.

@vercel
Copy link

vercel bot commented Aug 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hardhat ✅ Ready (Inspect) Visit Preview Aug 27, 2022 at 4:27PM (UTC)
hardhat-storybook ✅ Ready (Inspect) Visit Preview Aug 27, 2022 at 4:27PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Aug 17, 2022

⚠️ No Changeset found

Latest commit: 9c6f28f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

This issue is also being tracked on Linear.

We use Linear to manage our development process, but we keep the conversations on Github.

LINEAR-ID: 06ae891e-a08f-4d58-b99a-8e90931bcd84

Copy link
Member

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

I will review this again later because I want to understand a bit better the runtime part, but this should work as an initial review.

packages/hardhat-core/src/types/runtime.ts Show resolved Hide resolved
packages/hardhat-core/src/internal/core/flamegraph.ts Outdated Show resolved Hide resolved
}

const children = toFold.children.map((c) => foldFramegraph(c));
children.sort((a, b) => (a.parallel === b.parallel ? 0 : 1));
Copy link
Member

Choose a reason for hiding this comment

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

This looks suspicious to me. It seems like the intention is to put all the parallel items together, but I'm pretty sure that's not what this code will do.

My other problem is that whatever order is set here will be broken by the next sort.

What would make sense to me would be to put all the parallel items at the beginning, and the non-parallel at the end, and to then sort each half by name:

children.sort((a, b) => {
  if (a.parallel === b.parallel) {
    return a.name.localeCompare(b.name);
  }
  return a.parallel ? -1 : 1;
});

But I don't know if this is what you had in mind.

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 intended behavior is similar to what you proposed. But take into account that Array.prototype.sort is a stable sort.

We first sort by parallel, then by name. So that all of the children with the same name are together, and within a name-group, the parallel ones are also together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please ping me on slack if you want to go over this together.

Copy link
Member

Choose a reason for hiding this comment

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

I understand this now. What I didn't understand was that the goal is to have the items grouped by name, and sorted internally by parallel, not the other way around (sorted by parallel state, and internally by name).

We need to fix the sort by parallel to return a -1 too though.

packages/hardhat-core/src/internal/cli/cli.ts Outdated Show resolved Hide resolved
Co-authored-by: Franco Victorio <victorio.franco@gmail.com>
@alcuadrado
Copy link
Member Author

I just answered and/or resolved all the comments

@alcuadrado alcuadrado merged commit 4738192 into compiler-optimizations Aug 27, 2022
@alcuadrado alcuadrado deleted the flamegraph-generation branch August 27, 2022 16:37
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants