From 7169660cc317d32e123c065e6945a3f51f6ebba7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 16 Feb 2026 19:56:06 +0000 Subject: [PATCH 1/2] Fix memory leaks and apply Rust best practices to NIF - Add shrink_to_fit() in decoder_decode_chunk_impl to release excess String capacity before Rustler copies into BEAM binary. Without this, streaming workloads over-allocate up to 3x per chunk due to max_utf8_buffer_length worst-case estimates. - Replace write_all() with copy_from_slice() in encode_impl and encode_batch, removing the std::io::Write import. copy_from_slice is infallible when lengths match (guaranteed by prior allocation). - Eliminate unnecessary String heap allocations in error paths: use String::new() in decode_impl/decode_batch (Elixir side discards the value), return &'static str from canonical_name and detect_bom since encoding_rs::Encoding::name() already returns &'static str. - Remove unused atoms (unknown_encoding, encode_error, decode_error, no_bom) from the atoms! macro. https://claude.ai/code/session_01Mg82oZeWczDyGDe5eoGXXj --- native/encoding_rs/src/lib.rs | 43 ++++++++++++++++------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/native/encoding_rs/src/lib.rs b/native/encoding_rs/src/lib.rs index 00cc25a..94759fa 100644 --- a/native/encoding_rs/src/lib.rs +++ b/native/encoding_rs/src/lib.rs @@ -19,17 +19,12 @@ use encoding_rs::Encoding; use rustler::{Atom, Binary, Env, NifResult, OwnedBinary, Resource, ResourceArc}; -use std::io::Write; use std::sync::Mutex; mod atoms { rustler::atoms! { ok, error, - unknown_encoding, - encode_error, - decode_error, - no_bom, } } @@ -74,7 +69,8 @@ fn decode_impl<'a>(_env: Env<'a>, in_binary: Binary, enc: &str) -> NifResult<(At // encoding_rs replaces unmappable characters with U+FFFD automatically Ok((atoms::ok(), decoded.into_owned())) } - None => Ok((atoms::error(), "unknown_encoding".to_string())), + // Empty string avoids heap allocation; Elixir side discards this value + None => Ok((atoms::error(), String::new())), } } @@ -105,18 +101,16 @@ fn encode_impl<'a>(env: Env<'a>, in_str: &str, enc: &str) -> NifResult<(Atom, Bi match Encoding::for_label(enc.as_bytes()) { Some(encoding) => { let (encoded, _, _had_errors) = encoding.encode(in_str); - // encoding_rs replaces unmappable characters automatically let mut bin = OwnedBinary::new(encoded.len()) .ok_or_else(|| rustler::Error::Term(Box::new("allocation_failed")))?; - bin.as_mut_slice() - .write_all(&encoded) - .map_err(|_| rustler::Error::Term(Box::new("write_failed")))?; + // copy_from_slice is infallible here: bin was allocated with encoded.len() + bin.as_mut_slice().copy_from_slice(&encoded); Ok((atoms::ok(), bin.release(env))) } None => { - // Return empty binary for error case + // OwnedBinary::new(0) is a zero-cost allocation for the error case let bin = OwnedBinary::new(0) .ok_or_else(|| rustler::Error::Term(Box::new("allocation_failed")))?; Ok((atoms::error(), bin.release(env))) @@ -146,10 +140,10 @@ fn encoding_exists(enc: &str) -> bool { /// * `{:ok, name}` with the canonical encoding name /// * `{:error, :unknown_encoding}` if not recognized #[rustler::nif] -fn canonical_name(enc: &str) -> (Atom, String) { +fn canonical_name(enc: &str) -> (Atom, &'static str) { match Encoding::for_label(enc.as_bytes()) { - Some(encoding) => (atoms::ok(), encoding.name().to_string()), - None => (atoms::error(), "unknown_encoding".to_string()), + Some(encoding) => (atoms::ok(), encoding.name()), + None => (atoms::error(), "unknown_encoding"), } } @@ -231,7 +225,8 @@ fn decode_batch(items: Vec<(Binary, &str)>) -> Vec<(Atom, String)> { let (decoded, _, _had_errors) = encoding.decode(binary.as_slice()); (atoms::ok(), decoded.into_owned()) } - None => (atoms::error(), "unknown_encoding".to_string()), + // Empty string avoids heap allocation; Elixir side discards this value + None => (atoms::error(), String::new()), }) .collect() } @@ -260,9 +255,7 @@ fn encode_batch<'a>(env: Env<'a>, items: Vec<(&str, &str)>) -> NifResult { @@ -293,10 +286,10 @@ fn encode_batch<'a>(env: Env<'a>, items: Vec<(&str, &str)>) -> NifResult (Atom, String, usize) { +fn detect_bom(data: Binary) -> (Atom, &'static str, usize) { match Encoding::for_bom(data.as_slice()) { - Some((encoding, bom_length)) => (atoms::ok(), encoding.name().to_string(), bom_length), - None => (atoms::error(), "no_bom".to_string(), 0), + Some((encoding, bom_length)) => (atoms::ok(), encoding.name(), bom_length), + None => (atoms::error(), "no_bom", 0), } } @@ -379,8 +372,8 @@ fn decoder_decode_chunk_impl( let input = chunk.as_slice(); - // Calculate maximum output size: worst case is 3 bytes per input byte for UTF-8 - // plus potential replacement characters + // max_utf8_buffer_length is a worst-case estimate (up to 3x input size). + // We shrink after decoding to avoid passing oversized buffers through Rustler. let max_output_len = decoder .max_utf8_buffer_length(input.len()) .unwrap_or(input.len() * 3 + 3); @@ -388,6 +381,10 @@ fn decoder_decode_chunk_impl( let mut output = String::with_capacity(max_output_len); let (_result, _read, had_errors) = decoder.decode_to_string(input, &mut output, is_last); + // Release excess capacity before Rustler copies this into a BEAM binary. + // Without this, a 1KB chunk could hold a 3KB+ buffer until GC collects it. + output.shrink_to_fit(); + Ok((atoms::ok(), output, had_errors)) } From 699cb147bd6fa561aec9aa60145551ad640edaf0 Mon Sep 17 00:00:00 2001 From: jeffhuen <32542276+jeffhuen@users.noreply.github.com> Date: Mon, 16 Feb 2026 14:50:50 -0800 Subject: [PATCH 2/2] Review fixes: extract empty_binary helper, fix comments, add shrink threshold - Extract empty_binary() helper to de-duplicate OwnedBinary::new(0) logic across encode_impl and encode_batch, with accurate doc comment explaining that enif_alloc_binary(0) is not zero-cost - Standardize error-path comments to reference Elixir normalize_result/1 - Add 4KB threshold to shrink_to_fit() in streaming decoder to avoid realloc overhead on small chunks where savings are negligible --- native/encoding_rs/src/lib.rs | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/native/encoding_rs/src/lib.rs b/native/encoding_rs/src/lib.rs index 94759fa..1e81abd 100644 --- a/native/encoding_rs/src/lib.rs +++ b/native/encoding_rs/src/lib.rs @@ -69,7 +69,7 @@ fn decode_impl<'a>(_env: Env<'a>, in_binary: Binary, enc: &str) -> NifResult<(At // encoding_rs replaces unmappable characters with U+FFFD automatically Ok((atoms::ok(), decoded.into_owned())) } - // Empty string avoids heap allocation; Elixir side discards this value + // Elixir normalize_result/1 discards the string in error tuples None => Ok((atoms::error(), String::new())), } } @@ -110,14 +110,23 @@ fn encode_impl<'a>(env: Env<'a>, in_str: &str, enc: &str) -> NifResult<(Atom, Bi Ok((atoms::ok(), bin.release(env))) } None => { - // OwnedBinary::new(0) is a zero-cost allocation for the error case - let bin = OwnedBinary::new(0) - .ok_or_else(|| rustler::Error::Term(Box::new("allocation_failed")))?; - Ok((atoms::error(), bin.release(env))) + let bin = empty_binary(env)?; + Ok((atoms::error(), bin)) } } } +/// Returns a zero-length BEAM binary for error-path returns. +/// +/// `enif_alloc_binary(0)` still performs allocator bookkeeping, so this is +/// not free, but it is the minimal allocation Rustler supports for a +/// `Binary<'a>` return. +fn empty_binary<'a>(env: Env<'a>) -> NifResult> { + let bin = + OwnedBinary::new(0).ok_or_else(|| rustler::Error::Term(Box::new("allocation_failed")))?; + Ok(bin.release(env)) +} + /// Checks if an encoding label is valid/supported. /// /// ## Arguments @@ -225,7 +234,7 @@ fn decode_batch(items: Vec<(Binary, &str)>) -> Vec<(Atom, String)> { let (decoded, _, _had_errors) = encoding.decode(binary.as_slice()); (atoms::ok(), decoded.into_owned()) } - // Empty string avoids heap allocation; Elixir side discards this value + // Elixir normalize_result/1 discards the string in error tuples None => (atoms::error(), String::new()), }) .collect() @@ -259,9 +268,8 @@ fn encode_batch<'a>(env: Env<'a>, items: Vec<(&str, &str)>) -> NifResult { - let bin = OwnedBinary::new(0) - .ok_or_else(|| rustler::Error::Term(Box::new("allocation_failed")))?; - Ok((atoms::error(), bin.release(env))) + let bin = empty_binary(env)?; + Ok((atoms::error(), bin)) } }) .collect() @@ -382,8 +390,11 @@ fn decoder_decode_chunk_impl( let (_result, _read, had_errors) = decoder.decode_to_string(input, &mut output, is_last); // Release excess capacity before Rustler copies this into a BEAM binary. - // Without this, a 1KB chunk could hold a 3KB+ buffer until GC collects it. - output.shrink_to_fit(); + // Skip for small buffers where the realloc overhead outweighs the savings. + let excess = output.capacity() - output.len(); + if excess > 4096 { + output.shrink_to_fit(); + } Ok((atoms::ok(), output, had_errors)) }