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

Use Uint8Array instead of Buffer #8

Merged
merged 5 commits into from
Jul 5, 2024
Merged

Use Uint8Array instead of Buffer #8

merged 5 commits into from
Jul 5, 2024

Conversation

bjornstar
Copy link
Contributor

Continuing to work on changing Buffer usage to Uint8Array, we got a pretty decent performance improvement out of this one:

Before

buffer: 8.211ms
  ✔ .buffer()
stream: 20.57ms
  ✔ .stream()
file: 11.752ms
  ✔ .file()
  ✔ .fileSync() (1s)
fileSync: 1.053s

After

buffer: 4.755ms
  ✔ .buffer()
stream: 15.74ms
  ✔ .stream()
file: 7.634ms
  ✔ .file()
  ✔ .fileSync() (600ms)
fileSync: 599.281ms

I pulled in uint8array-extras for the indexOf function and used that instead of looping over every byte in the buffer one by one.

I bumped the minimum version of node to 18 to match read-chunk's requirement. This is great because node v16 has an annoying limitation when a buffer references its own values.

I added two new files to the fixtures that are over 65535 bytes so we can test to ensure streaming works across multiple chunks. It is CC licensed and I added a link to the license file.

Also bumped devDependencies and updated the workflows to modern versions.

@sindresorhus
Copy link
Owner

sindresorhus commented Jul 5, 2024

It is CC licensed and I added a link to the license file.

I don't see the need to bring in a CC licensed file. Just pick one from Unsplash (no license / credit needed).

index.d.ts Outdated

declare const isProgressive: {
/**
Checks if a `Buffer` contains a JPEG image that is [progressive](http://www.faqs.org/faqs/jpeg-faq/part1/section-11.html).
Checks if an `ArrayBuffer/UInt8Array` contains a JPEG image that is [progressive](http://www.faqs.org/faqs/jpeg-faq/part1/section-11.html).
Copy link
Owner

Choose a reason for hiding this comment

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

The type only indicates it supports UInt8Array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to Uint8Array, plus I keep capitalizing UInt instead of Uint, bad habit.

@@ -5,26 +5,34 @@ import isProgressive from './index.js';

const getPath = name => `fixture/${name}.jpg`;

test('.buffer()', t => {
test.serial('.buffer()', t => {
Copy link
Owner

Choose a reason for hiding this comment

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

why serial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're reading the same files as the other tests that are run in parallel. This blocks the reading of the files in the other tests making them slower or causes hangs where the file cannot be opened because it's being accessed asynchronously.

I benchmarked the methods by running each test multiple times and could only get it to behave consistently if the synchronous tests are run serially.

Copy link
Owner

Choose a reason for hiding this comment

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

👍

@bjornstar
Copy link
Contributor Author

It is CC licensed and I added a link to the license file.

I don't see the need to bring in a CC licensed file. Just pick one from Unsplash (no license / credit needed).

Ah, thanks for the tip, I'll swap it out.

@sindresorhus sindresorhus merged commit 7d35ca0 into sindresorhus:main Jul 5, 2024
2 checks passed
@sindresorhus
Copy link
Owner

Thanks 🙏

@bjornstar bjornstar deleted the goodbye-nodejs-buffer branch July 5, 2024 11:13
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.

2 participants