Merge pull request #375 from napi-rs/external-buffer-memory-issue

fix(napi): memory issues in create_external_buffer

bench/Cargo.toml

@@ -11,6 +11,12 @@ crate-type = ["cdylib"]
 napi = {path = "../napi"}
 napi-derive = {path = "../napi-derive"}
 
+[target.'cfg(all(unix, not(target_env = "musl"), not(target_arch = "aarch64")))'.dependencies]
+jemallocator = {version = "0.3", features = ["disable_initial_exec_tls"]}
+
+[target.'cfg(windows)'.dependencies]
+mimalloc = {version = "0.1"}
+
 [build-dependencies]
 napi-build = {path = "../build"}
 

bench/bench.ts

@@ -4,11 +4,17 @@ import { join } from 'path'
 import { Summary } from 'benny/lib/internal/common-types'
 
 import { benchAsync } from './async'
+import { benchBuffer } from './buffer'
 import { benchNoop } from './noop'
 import { benchPlus } from './plus'
 
 async function run() {
-  const output = [await benchNoop(), await benchPlus(), await benchAsync()]
+  const output = [
+    await benchNoop(),
+    await benchPlus(),
+    await benchBuffer(),
+    await benchAsync(),
+  ]
     .map(formatSummary)
     .join('\n')
 

bench/buffer.ts

@@ -0,0 +1,23 @@
+import b from 'benny'
+
+const { benchCreateBuffer } = require('./index.node')
+
+function createBuffer() {
+  const buf = Buffer.alloc(100000)
+  buf[0] = 1
+  buf[1] = 2
+  return buf
+}
+
+export const benchBuffer = () =>
+  b.suite(
+    'Create buffer',
+    b.add('napi-rs', () => {
+      benchCreateBuffer()
+    }),
+    b.add('JavaScript', () => {
+      createBuffer()
+    }),
+    b.cycle(),
+    b.complete(),
+  )

bench/src/buffer.rs

@@ -0,0 +1,16 @@
+use napi::{ContextlessResult, Env, JsBuffer, JsObject, Result};
+
+#[contextless_function]
+pub fn bench_create_buffer(env: Env) -> ContextlessResult<JsBuffer> {
+  let mut output = Vec::with_capacity(100000);
+  output.push(1);
+  output.push(2);
+  env
+    .create_buffer_with_data(output)
+    .map(|v| Some(v.into_raw()))
+}
+
+pub fn register_js(exports: &mut JsObject) -> Result<()> {
+  exports.create_named_method("benchCreateBuffer", bench_create_buffer)?;
+  Ok(())
+}

bench/src/lib.rs

@@ -3,7 +3,16 @@ extern crate napi_derive;
 
 use napi::{JsObject, Result};
 
+#[cfg(all(unix, not(target_env = "musl"), not(target_arch = "aarch64")))]
+#[global_allocator]
+static ALLOC: jemallocator::Jemalloc = jemallocator::Jemalloc;
+
+#[cfg(windows)]
+#[global_allocator]
+static ALLOC: mimalloc::MiMalloc = mimalloc::MiMalloc;
+
 mod async_compute;
+mod buffer;
 mod noop;
 mod plus;
 
@@ -12,6 +21,7 @@ fn init(mut exports: JsObject) -> Result<()> {
   exports.create_named_method("noop", noop::noop)?;
 
   async_compute::register_js(&mut exports)?;
+  buffer::register_js(&mut exports)?;
   plus::register_js(&mut exports)?;
 
   Ok(())

bench/tsconfig.json

@@ -1,7 +1,8 @@
 {
   "extends": "../tsconfig.json",
   "compilerOptions": {
-    "target": "ESNext"
+    "target": "ESNext",
+    "rootDir": "."
   },
   "include": ["."],
   "exclude": ["target", "src"]

napi/src/env.rs

@@ -284,7 +284,7 @@ impl Env {
   /// This API allocates a node::Buffer object and initializes it with data backed by the passed in buffer.
   /// While this is still a fully-supported data structure, in most cases using a TypedArray will suffice.
   pub fn create_buffer_with_data(&self, mut data: Vec<u8>) -> Result<JsBufferValue> {
-    let mut length = data.len();
+    let length = data.len();
     let mut raw_value = ptr::null_mut();
     let data_ptr = data.as_mut_ptr();
     check_status!(unsafe {
@@ -293,7 +293,7 @@ impl Env {
         length,
         data_ptr as *mut c_void,
         Some(drop_buffer),
-        &mut length as *mut usize as *mut _,
+        Box::into_raw(Box::new((length, data.capacity()))) as *mut c_void,
         &mut raw_value,
       )
     })?;
@@ -403,7 +403,7 @@ impl Env {
 
   #[inline]
   pub fn create_arraybuffer_with_data(&self, data: Vec<u8>) -> Result<JsArrayBufferValue> {
-    let mut length = data.len();
+    let length = data.len();
     let mut raw_value = ptr::null_mut();
     let data_ptr = data.as_ptr();
     check_status!(unsafe {
@@ -412,7 +412,7 @@ impl Env {
         data_ptr as *mut c_void,
         length,
         Some(drop_buffer),
-        &mut length as *mut usize as *mut c_void,
+        Box::into_raw(Box::new((length, data.capacity()))) as *mut c_void,
         &mut raw_value,
       )
     })?;
@@ -1023,10 +1023,14 @@ impl Env {
   }
 }
 
-unsafe extern "C" fn drop_buffer(env: sys::napi_env, finalize_data: *mut c_void, len: *mut c_void) {
-  let length = len as *mut u64;
-  let length = *length as usize;
-  let _ = Vec::from_raw_parts(finalize_data as *mut u8, length, length);
+unsafe extern "C" fn drop_buffer(
+  env: sys::napi_env,
+  finalize_data: *mut c_void,
+  hint: *mut c_void,
+) {
+  let length_ptr = hint as *mut (usize, usize);
+  let (length, cap) = *Box::from_raw(length_ptr);
+  mem::drop(Vec::from_raw_parts(finalize_data as *mut u8, length, cap));
   let mut changed = 0;
   let adjust_external_memory_status =
     sys::napi_adjust_external_memory(env, -(length as i64), &mut changed);