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

perf(fs): use realpathSync native for >15%. improvement [HH-446] #2522

Closed

Conversation

sambacha
Copy link
Contributor

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
  • I didn't do anything of this.

Performance: Faster Load Time with realPathSync.native

TypeScript now leverages a system-native implementation of the Node.js realPathSync function on all operating systems.

Previously this function was only used on Linux, but in TypeScript 4.5 it has been adopted to operating systems that are typically case-insensitive, like Windows and MacOS. On certain codebases, this change sped up project loading by 5-13% (depending on the host operating system).

Reference material

For more information, see the original change here, along with the 4.5-specific changes here.

ref: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/#real-path-sync-native

Discussion

This is a rough cut, would like some feedback if you guys are interested in these possible changes (it does enforce at least typescript 4.5, and you are using ~ for semver of typescript)

Additionally, changing how tmpDir is managed with instead mkdtmp the nodejs utility is something I observed as a possible easy change that would yield very slight gain (potentially). Another would be instead of downloading repeatedly solidity compilers offer a global config option (I am sure users would do this) + offer a more compressed binary for solc native (quicker downloads). etc etc.

Notes

Originally scoped out files

packages/hardhat-core/test/helpers/project.ts
packages/hardhat-core/test/internal/hardhat-network/stack-traces/test.ts
packages/hardhat-core/test/internal/hardhat-network/stack-traces/compilation.ts
packages/hardhat-core/test/internal/util/download.ts
packages/hardhat-core/test/internal/util/packageInfo.ts
packages/hardhat-core/test/internal/core/typescript-support.ts
packages/hardhat-core/test/internal/solidity/helpers.ts
packages/hardhat-core/test/internal/solidity/dependencyGraph.ts
packages/hardhat-core/src/builtin-tasks/utils/solidity-files-cache.ts

Reference fs.([1], :[2])

fs.link(srcpath, dstpath, callback);             // Asynchronous link. No arguments other than a possible exception are given to the completion callback.
fs.linkSync(srcpath, dstpath);                   // Synchronous link.
fs.symlink(srcpath, dstpath, [type], callback);  // Asynchronous symlink. No arguments other than a possible exception are given to the completion callback. The type argument can be set to 'dir', 'file', or 'junction' (default is 'file') and is only available on Windows (ignored on other platforms)
fs.symlinkSync(srcpath, dstpath, [type]);        // Synchronous symlink.
fs.readlink(path, callback);                     // Asynchronous readlink. The callback gets two arguments (err, linkString).
fs.readlinkSync(path);                           // Synchronous readlink. Returns the symbolic link's string value.
fs.unlink(path, callback);                       // Asynchronous unlink. No arguments other than a possible exception are given to the completion callback.
fs.unlinkSync(path);                             // Synchronous unlink.

fs.realpath(path, [cache], callback);     // Asynchronous realpath. The callback gets two arguments (err, resolvedPath).
fs.realpathSync(path, [cache]);           // Synchronous realpath. Returns the resolved path.

@changeset-bot
Copy link

changeset-bot bot commented Mar 23, 2022

⚠️ No Changeset found

Latest commit: b083176

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

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sambacha sambacha changed the title perf(fs): use realpathSync native for >30%. improvement perf(fs): use realpathSync native for >15%. improvement Mar 23, 2022
@sambacha
Copy link
Contributor Author

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

This CLA covers any potential contribution to Nomic that does not necessarily include hardhat?

@fvictorio
Copy link
Member

This looks really interesting, thanks a lot @sambacha, we'll take a look as soon as we can.

This CLA covers any potential contribution to Nomic that does not necessarily include hardhat?

I'm not really sure. Tagging @alcuadrado that knows this stuff.

@fvictorio fvictorio changed the title perf(fs): use realpathSync native for >15%. improvement perf(fs): use realpathSync native for >15%. improvement [HH-446] Mar 23, 2022
@alcuadrado
Copy link
Member

I've just removed the CLA bot. Now that we relicensed everything to MIT it's no longer necessary.

@sambacha
Copy link
Contributor Author

Thanks guys, also would you be interested in leveraging Turborepo? I have another branch with that setup to build the entire repository. It is much faster for building the monorepo, should check it out imho

@sambacha
Copy link
Contributor Author

Also btw thanks for addressing my CLA question, makes contributing much easier

@fvictorio
Copy link
Member

Thanks guys, also would you be interested in leveraging Turborepo? I have another branch with that setup to build the entire repository. It is much faster for building the monorepo, should check it out imho

That's really interesting, we'll take a look. Thanks for the tip!

@alcuadrado
Copy link
Member

There's something weird going on with github actions here. I think this PR was created when it was down

@alcuadrado
Copy link
Member

Can you add an empty commit or give me access to push to your branch, @sambacha? Thanks!

@sambacha
Copy link
Contributor Author

sambacha commented Mar 24, 2022

Can you add an empty commit or give me access to push to your branch, @sambacha? Thanks!

@alcuadrado

I have added you as a collaborator with admin priv. to the repo!

Will fixup this PR now that I know the typescript versioning is not an issue

@alcuadrado
Copy link
Member

I've been reading more about this, and it's a great idea! realpathSync.native returns the path with the casing as it is on disk, so we don't need to use true-case-path anymore.

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2022

This PR was marked as stale because it didn't have any activity in the last 30 days. Please excuse us if we didn't have enough time to review it and get it merged. If you are still interested in getting these changes applied, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 6, 2022
@github-actions
Copy link
Contributor

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this May 13, 2022
@fvictorio fvictorio reopened this May 18, 2022
@github-actions
Copy link
Contributor

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this May 25, 2022
@fvictorio fvictorio reopened this May 26, 2022
@fvictorio fvictorio added not-stale and removed Stale labels May 26, 2022
@alcuadrado
Copy link
Member

Replaced by #3053. Thanks again, @sambacha

@alcuadrado alcuadrado closed this Aug 29, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 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