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.

Sign up for GitHub

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 severe node performance issue due to buffer allocation bug, fix undefined behavior which breaks node > 18.7.0 #82

Open
wants to merge 3 commits into
base: latest
Choose a base branch
from
Open

Fix severe node performance issue due to buffer allocation bug, fix undefined behavior which breaks node > 18.7.0 #82

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03254fe
Change to using std::min() when allocating Buffers on the C++ side. …
doggkruse Jun 27, 2022
f8a991c
Handle case where the previously allocated array buffer is shorter th…
doggkruse Jun 27, 2022
9eee8df
src/binding.cc:WrapPointer(): Fix bug in Node.js > 18.7.0, ensure len…
afreer Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/binding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,12 @@ class InstanceData final : public RefNapi::Instance {

auto it = pointer_to_orig_buffer.find(ptr);
if (it != pointer_to_orig_buffer.end()) {
it->second.finalizer_count++;
if (!it->second.ab.Value().IsEmpty()) {
// Already have a valid entry, nothing to do.
return;
}
it->second.ab.Reset(buf, 0);
it->second.finalizer_count++;
} else {
pointer_to_orig_buffer.emplace(ptr, ArrayBufferEntry {
Reference<ArrayBuffer>::New(buf, 0),
Expand Down Expand Up @@ -140,8 +140,8 @@ class InstanceData final : public RefNapi::Instance {
if (it != pointer_to_orig_buffer.end())
ab = it->second.ab.Value();

if (ab.IsEmpty()) {
length = std::max<size_t>(length, kMaxLength);
if (ab.IsEmpty() || (ab.ByteLength() < length)) {
length = std::min<size_t>(length, kMaxLength);
ab = Buffer<char>::New(env, ptr, length, [this](Env env, char* ptr) {
UnregisterArrayBuffer(ptr);
}).ArrayBuffer();
Expand All @@ -166,8 +166,15 @@ class InstanceData final : public RefNapi::Instance {
*/

Value WrapPointer(Env env, char* ptr, size_t length) {
if (ptr == nullptr)
if (ptr == nullptr) {
length = 0;
} else if (length == 0) {
// If length is 0 N-API doesn't guarantee it will save/restore ptr normally.
// For example, see https://nodejs.org/api/n-api.html#napi_get_typedarray_info
// "[out] data: ... If the length of the array is 0, this may be NULL or any
// other pointer value."
length = 1;
}

InstanceData* data;
if (ptr != nullptr && (data = InstanceData::Get(env)) != nullptr) {
Expand Down