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): realpath native support #2747

Closed
wants to merge 6 commits into from

Conversation

sambacha
Copy link
Contributor

@sambacha sambacha commented May 24, 2022

Subsantiave changes are documented below:

- import path from "path";
+ import path from 'node:path';



- : files.map((f) => fs.realpathSync(f));
+ : files.map((f) => fs.realpathSync.native(f));


- const configFile = fs.realpathSync(userConfigPath);
+ const configFile = fs.realpathSync.native(userConfigPath);

- fs.realpathSync(resolvedPackageJson) === fs.realpathSync(thisPackageJson)
+ fs.realpathSync.native(resolvedPackageJson) === fs.realpathSync.native(thisPackageJson)



+  // TODO: 
+ fs.mkdtempSync(path.join(os.tmpdir(), `hardhat-tests-${nameHint}`));
- const tmpDir = path.join(tmpDirContainer, `hardhat-tests-${nameHint}`);


+ [path.basename(s)]: { content: fs.realpathSync.native(s, "utf8") },
- [path.basename(s)]: { content: fs.readFileSync(s, "utf8") },


+ projectRoot = fs.realpathSync.native(".");
- projectRoot = fs.realpathSync(".");


+ const projectRoot = fs.realpathSync.native(".");
- const projectRoot = fs.realpathSync(".");


@@ packages/hardhat-core/test/internal/solidity/resolver.ts
+ const realpathSync = fs.realpathSync.native ?? fs.realpathSync;



- fs.realpathSync(path.join("contracts", "fromContracts.sol"))
+ fs.realpathSync.native(path.join("contracts", "fromContracts.sol"))

@changeset-bot
Copy link

changeset-bot bot commented May 24, 2022

⚠️ No Changeset found

Latest commit: 9cc752b

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: 1332152d-044f-4f8f-9880-9b2b935cdc71

@sambacha
Copy link
Contributor Author

Note this requires NodeJS 14, and typescript 4.6.3

@sambacha
Copy link
Contributor Author

the build was previously failing, fixed that now

@sambacha
Copy link
Contributor Author

Workflow changes

  • Changes made to workflows because this is a breaking change in typescript versioning and besides I do not think yall officially support nodejs 12 anymore.

  • by specifying --prefer-offline, yarn will check the cache first, and only try to download the package from the registry if it cannot find it in the cache.

  • updated the nodejs action to v3

Dependency changes

  • typescript@^4.6.3 (4.7.1 is latest)
  • I added turbo by accident , will remove that ofc.

Notes

Completely unsolicited ideas, I think some might be ez wins others, well YMMV - would like to know your thoughts ofc!

  1. Include certain compiler versions (e.g. 0.6.12) or pre-compiled contracts (OZ, Uni, etc) with hardhat potentially. For reference solc-typed-ast compiler does include a cache env for including compilers, see https://github.com/ConsenSys/solc-typed-ast/blob/master/docker/download.sh

  2. Nx Distributed Task Execution (DTE) Example/Benchmark, https://github.com/vsavkin/interstellar

  3. tree diffing when restoring files from the cache https://github.com/vsavkin/large-monorepo#why-is-nx-faster-than-turbo

  4. core_d.js launches a server so that your services invoke method is called with the same arguments. Later calls to invoke will be executed on the same instance.

3 may be useful for packages/hardhat-core/src/internal/util/scripts-runner.ts

  1. potentially invoking nodejs directly e.g.
node --expose-gc node_modules/.bin/jest --coverage --globals "{\"coverage\":true}" src/
  1. Utilize a more performant cache system like https://github.com/next-boost/hybrid-disk-cache - see why sqlite3 is faster than the filesystem (below certain file sizes ref: https://www.sqlite.org/fasterthanfs.html

Questions

How to best benchmark this? What sort of test would actually show any statistically significant differences? I would think maybe measuring access time or something as opposed to measuring compilation time.

@alcuadrado
Copy link
Member

Lots to digest here. Thanks @sambacha! What's the part that requires node 14?

Also, are the node: imports faster?

@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 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 Jul 3, 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 Jul 10, 2022
@fvictorio fvictorio reopened this Jul 11, 2022
@vercel
Copy link

vercel bot commented Jul 11, 2022

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

Name Status Preview Updated
hardhat ❌ Failed (Inspect) Jul 11, 2022 at 11:31AM (UTC)
hardhat-storybook ❌ Failed (Inspect) Jul 11, 2022 at 11:31AM (UTC)

@alcuadrado
Copy link
Member

This took some time, but we finally got to optimize the compilation process a lot. You can take a look at #3048 to #3053.

Thanks for these ideas <3

@alcuadrado alcuadrado closed this Aug 22, 2022
@alcuadrado
Copy link
Member

@gitpoap-bot @sambacha deserves a gitpoap, or a hundred actually

@alcuadrado
Copy link
Member

@gitpoap-bot @sambacha deserves a gitpoap

(second try)

@gitpoap-bot
Copy link

gitpoap-bot bot commented Nov 1, 2022

Congrats, @sambacha ! You've earned a GitPOAP for your contribution!

GitPOAP: 2022 Hardhat Contributor:

GitPOAP: 2022 Hardhat Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2023
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