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

Refactor: handle view encoding (Uint8Array) natively #43

Merged
merged 2 commits into from
Jun 19, 2022
Merged

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Jun 17, 2022

Replaces the internal asBuffer option with an encoding option that can be one of buffer, utf8 or view, and converts data accordingly. No longer need to transcode view to buffer.

Has no performance effect on get() with utf8, buffer or view encoding:

Click to expand

get 1655539636853
get 1655540192223
get 1655540226332

Same for iterator() (main is slightly faster at utf8 in this graph, but on average there's no difference):

Click to expand

iterate 1655540600711

@vweevers vweevers added benchmark Requires or pertains to benchmarking semver-patch Bug fixes that are backward compatible labels Jun 17, 2022
Replaces the internal asBuffer option with an encoding option that
can be one of buffer, utf8 or view, and converts data accordingly.
No longer have to transcode view to buffer.
@vweevers vweevers marked this pull request as ready for review June 18, 2022 08:38
@vweevers
Copy link
Member Author

I also tried napi_create_typedarray() which I expected to be faster for skipping a copy but it's actually a lot slower... Same goes for Buffer when using napi_create_external_buffer() instead of napi_create_buffer_copy(). I didn't dive into why it's slower, so here's the code for future reference:

void Convert2 (napi_env env, char* data, const size_t size, const Encoding encoding, napi_value& result) {
  if (data == NULL) {
    napi_get_undefined(env, &result);
  } else if (encoding == Encoding::buffer) {
    napi_create_external_buffer(env, size, data, NULL, NULL, &result);

    // Delete data once ArrayBuffer is garbage collected
    napi_value arrayBuffer;
    napi_get_named_property(env, result, "buffer", &arrayBuffer);
    napi_wrap(env, arrayBuffer, data, FinalizeArrayBuffer, NULL, NULL);
  } else {
    napi_value arrayBuffer;
    napi_create_external_arraybuffer(env, data, size, FinalizeArrayBuffer, NULL, &arrayBuffer);
    napi_create_typedarray(env, napi_typedarray_type::napi_uint8_array, size, arrayBuffer, 0, &result);
  }
}

void FinalizeArrayBuffer (napi_env env, void* data, void* hint) {
  if (data) {
    delete (char*) data;
  }
}

@vweevers vweevers removed the benchmark Requires or pertains to benchmarking label Jun 18, 2022
@vweevers vweevers changed the title Handle view encoding (Uint8Array) natively Refactor: handle view encoding (Uint8Array) natively Jun 19, 2022
@vweevers vweevers merged commit b9fd5e9 into main Jun 19, 2022
@vweevers vweevers deleted the native-view branch June 19, 2022 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Bug fixes that are backward compatible
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant