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

fix: Speed up cache reads #255

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

thecodrr
Copy link
Contributor

This PR speeds up read.stream and read by skipping fs.stat call if size was passed via opts. Currently, the only reason for doing a stat call is to get the size (and throw the size mismatch error if the size is different). This is unnecessary for 3 reasons:

  1. In the case of read.stream, the stream already compares the sizes at the end and throws an error if there's a mismatch.
  2. In the case of read, we can compare the sizes after reading the cache contents
  3. In both cases we are already doing an integrity check which would automatically fail if there's a size difference since the hashes would be different.

In this PR, the stat call is only made if the user does not pass a size property via opts. This makes sense because without knowing the size, the stream has to make an unnecessary fs.read call at the end before closing which has a significant cost (that cost is much, much greater than the cost of doing fs.stat).

On my machine, the benchmarks with this change look like this:

┌─────────┬─────────────────────┬─────────┬────────────────────┬───────────┬─────────┐
│ (index) │      Task Name      │ ops/sec │ Average Time (ns)  │  Margin   │ Samples │
├─────────┼─────────────────────┼─────────┼────────────────────┼───────────┼─────────┤
│    0    │ 'read.stream (new)' │ '4,643' │ 215352.03841424757 │ '±10.20%' │   465   │
│    1    │ 'read.stream (old)' │ '3,933' │ 254237.5665025663  │ '±7.17%'  │   394   │
│    2    │    'read (old)'     │ '2,915' │ 343045.55719845917 │ '±13.42%' │   292   │
│    3    │    'read (new)'     │ '4,392' │ 227636.30011033904 │ '±12.14%' │   449   │
└─────────┴─────────────────────┴─────────┴────────────────────┴───────────┴─────────┘

That's a solid 16% improvement in the case of read.stream and 36% improvement in the case of read.

References

@thecodrr thecodrr requested a review from a team as a code owner December 24, 2023 11:05
@wraithgar wraithgar changed the title Speed up cache reads fix: Speed up cache reads Jan 3, 2024
@@ -55,13 +57,10 @@ function readStream (cache, integrity, opts = {}) {
// Set all this up to run on the stream and then just return the stream
Promise.resolve().then(async () => {
const { stat, cpath, sri } = await withContentSri(cache, integrity, async (cpath, sri) => {
// just stat to ensure it exists
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this comment should have been a code smell. It's right in the docs for fs.stat

Using fs.stat() to check for the existence of a file before calling fs.open(), fs.readFile(), or fs.writeFile() is not recommended. Instead, user code should open/read/write the file directly and handle the error raised if the file is not available.

@wraithgar wraithgar merged commit b9f488d into npm:main Jan 3, 2024
15 of 16 checks passed
@github-actions github-actions bot mentioned this pull request Jan 3, 2024
@thecodrr thecodrr deleted the faster-cache-read branch January 4, 2024 14:18
wraithgar pushed a commit that referenced this pull request Jan 4, 2024
🤖 I have created a release *beep* *boop*
---


## [18.0.2](v18.0.1...v18.0.2)
(2024-01-03)

### Bug Fixes

*
[`b9f488d`](b9f488d)
[#255](#255) Speed up cache reads
(#255) (@thecodrr)

### Chores

*
[`7eab139`](7eab139)
[#252](#252) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`44bedb2`](44bedb2)
[#252](#252) bump
@npmcli/template-oss from 4.21.1 to 4.21.3 (@dependabot[bot])
*
[`a12bdf3`](a12bdf3)
[#248](#248) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`62e5a94`](62e5a94)
[#248](#248) bump
@npmcli/template-oss from 4.19.0 to 4.21.1 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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

2 participants