-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Use Uint8Array instead of Buffer #8
Conversation
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). |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why serial?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Ah, thanks for the tip, I'll swap it out. |
Thanks 🙏 |
Continuing to work on changing Buffer usage to Uint8Array, we got a pretty decent performance improvement out of this one:
Before
After
I pulled in
uint8array-extras
for theindexOf
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.