Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

feat: improve performance by reworking caching and supply cache to NFT #1244

Merged
merged 8 commits into from
Nov 4, 2022

Conversation

danez
Copy link
Contributor

@danez danez commented Nov 1, 2022

Summary

Based on #1243

Ref: https://github.com/netlify/pod-compute/issues/252

There is also another PR that I created in NFT, as NFT does not correctly cache, which will bring another small performance boost. vercel/nft#320


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻.
    This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing
    a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@danez danez changed the title Perf feat: improve performance by reworking caching and supply cache to NFT Nov 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2022

⏱ Benchmark results

largeDepsEsbuild: 2.1s

^   2.1s  
│   ┌──┐  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
└───┴──┴──>
     T    
Legend
  • T (current commit): 2.1s

largeDepsNft: 9.4s

^   9.4s  
│   ┌──┐  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
└───┴──┴──>
     T    
Legend
  • T (current commit): 9.4s

largeDepsZisi: 14.5s

^  14.5s  
│   ┌──┐  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
└───┴──┴──>
     T    
Legend
  • T (current commit): 14.5s

// loses part of the return type information. We can safely say it's a
// Buffer in this case because we're not specifying an encoding.
const binaryInfo = detect(buffer as Buffer)
const fileContents = await readFile(path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the cache here, as this is reading a binary file and we only do that once for a binary we find in the function folder.


import { isNativeModule } from '../../utils/detect_native_module.js'
import { PackageJson } from '../../utils/package_json.js'
import { PackageJson, readPackageJson } from '../../utils/package_json.js'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

read-package-json-fast does JSON.parse(readFile()) plus some additional validation and checks for binary folders. So our implementation should actually be faster without those checks. :)

@danez danez marked this pull request as ready for review November 2, 2022 13:59
@danez danez requested a review from a team November 2, 2022 13:59
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

I love the idea of having a single cache entity that is used for IO operations and for NFT! ✨

I left a few comments, but looks great in general.

src/utils/cache.ts Outdated Show resolved Hide resolved
src/runtimes/node/bundlers/nft/es_modules.ts Show resolved Hide resolved
src/runtimes/node/bundlers/nft/index.ts Outdated Show resolved Hide resolved
src/utils/cache.ts Outdated Show resolved Hide resolved
src/utils/cache.ts Outdated Show resolved Hide resolved
@danez danez requested a review from eduardoboucas November 3, 2022 12:01
eduardoboucas
eduardoboucas previously approved these changes Nov 3, 2022
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

LGTM! This change is significant enough to warrant a feature flag, but I'm not sure how easy it would be to add one, since the changes have pretty deep ramifications.

Have you thought about that?

@danez
Copy link
Contributor Author

danez commented Nov 3, 2022

LGTM! This change is significant enough to warrant a feature flag, but I'm not sure how easy it would be to add one, since the changes have pretty deep ramifications.

Have you thought about that?

Yes I forgot about this. I put the nft cache and the higher file IO limit behind FF. The rest was more or less a refactor and should not change much.

eduardoboucas
eduardoboucas previously approved these changes Nov 3, 2022
@danez danez merged commit 22725d7 into main Nov 4, 2022
@danez danez deleted the perf branch November 4, 2022 10:46
Skn0tt pushed a commit to netlify/build that referenced this pull request May 21, 2024
netlify/zip-it-and-ship-it#1244)

* feat: improve performance by reworking caching and supply cache to NFT

* chore: move cache into class

* chore: comment about fileIOConcurrency

* Update src/runtimes/node/bundlers/nft/es_modules.ts

Co-authored-by: Eduardo Bouças <[email protected]>

* chore: fix comment

* chore: fix comment

* chore: add ff

Co-authored-by: Eduardo Bouças <[email protected]>
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.

2 participants