Skip to content

feat: support buffers in write() and sendFeatureReport() - #357

Merged
todbot merged 1 commit into
node-hid:masterfrom
Julusian:feat/buffers
Dec 16, 2019
Merged

todbot merged 1 commit into
node-hid:masterfrom
Julusian:feat/buffers

Conversation

@Julusian

@Julusian Julusian commented Dec 15, 2019

Copy link
Copy Markdown
Collaborator

This is an alternative to #171 (implementation of #122)

https://nodejs.org/api/buffer.html#buffer_buffer

Instances of the Buffer class are similar to arrays of integers from 0 to 255 (other integers are coerced to this range by & 255 operation) but correspond to fixed-sized, raw memory allocations outside the V8 heap.

So a buffer is essentially a byte array. Using them can bring a noticable performance boost to write calls due to not needing to validate the payload much as the elements must already be numbers and within range.

I protoyped this (before finding the issue and pr above) while testing with node-elgato-stream-deck
In that, the write calls are 1kb or 8kb each. In the 1kb case, I am seeing calls to write being about 33% quicker.
For ~150 calls it is down from 120-150ms to 70-100ms.

With the model sending 8kb packets, for 30 calls to write the time taken decreases from ~115ms to 70ms.

Throwing some timing measurements into the c++, shows that the number array to char array conversion takes 100-400µs per 1kb, vs 0-4µs when taking a buffer

@todbot

todbot commented Dec 15, 2019

Copy link
Copy Markdown
Contributor

Hi!
This looks great, thank you. Does this maintain backwards compatibility for existing users who pass in arrays?

@Julusian

Copy link
Copy Markdown
Collaborator Author

Yes it does. I have been testing with both to check I didnt break anything and to check the performance difference.

@todbot

todbot commented Dec 16, 2019

Copy link
Copy Markdown
Contributor

That's awesome. Normally HID APIs don't concern themselves much with performance because the underlying systems (and USB HID itself) are slow compared to other USB protocols. But I'm glad some efficiencies can be had.

I'll go ahead and merge this assuming everything works, do some tests on my end, and hopefully have a new release in a few days.

Thanks!

@todbot
todbot merged commit 1d274b7 into node-hid:master Dec 16, 2019
@Julusian

Copy link
Copy Markdown
Collaborator Author

Thanks.
Yeah this does feel like a bit of an abuse of the hid protocol, so I'm not surprised it is a bit slow.

This does still feel a bit sluggish for transferring icons for streamdeck keys, and has a noticable rolling effect when filling the whole panel, so I might be back later if I find any other gains that don't create a mess.

@Julusian

Julusian commented Jan 7, 2020

Copy link
Copy Markdown
Collaborator Author

@todbot What is the status of a release with this fix included? Just checking it hasn't been forgotten, I can wait if you haven't had the time :)

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