feat(bindings/bls): use external buffers for blst operations - #356
feat(bindings/bls): use external buffers for blst operations#356spiral-ladder wants to merge 1 commit into
Conversation
This is one of possible likely causes for increased GC pressure on experiments to swap out blst-ts for lodestar-z/bls, as observed on feat2 and feat3 deployments in [this PR](ChainSafe/lodestar#9342). With external array buffers, V8 is only aware of the pointer to the backing memory, instead of having to track both the pointer and the backing memory. This means that during marking phase the GC does not have to walk the backing memory to mark it as 'live' - the frequency of the GC firing off is still the same, but each cycle does less work. This of course comes with a tradeoff, we need a **finalizer** to let V8 know how much external memory is in native heap so that the GC tells the native impl to free the useless memory. Though, regardless of the effect, we should still probably do this anyway, since [napi-rs does the same](https://github.com/napi-rs/napi-rs/blob/159395b365c583a6642ad481edc5708d9f36a24b/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs#L175), and only defaults to V8 managed array buffers if it is disallowed (like in Electron).
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes memory management for blst operations by transitioning from V8-managed array buffers to external array buffers. By moving the backing memory to the native heap, the garbage collector no longer needs to traverse these buffers during marking phases, which helps mitigate GC pressure in high-throughput environments. A native finalizer has been added to ensure proper memory cleanup, maintaining safety while improving performance. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a createExternalUint8Array helper and a corresponding finalizer in bindings/napi/blst.zig to manage external memory for JavaScript Uint8Array objects, refactoring the byte conversion logic for public keys, signatures, and secret keys. Feedback focuses on adhering to the repository style guide, specifically regarding the inclusion of assertions for arguments and return values, renaming helper functions for clarity, avoiding architecture-specific types like usize, enforcing line length limits, and ensuring robust error handling.
| fn createExternalUint8Array(env: napi.Env, bytes: []const u8) !napi.Value { | ||
| const buf = try allocator.alloc(u8, bytes.len); | ||
| @memcpy(buf, bytes); | ||
|
|
||
| const len_hint: ?*anyopaque = @ptrFromInt(bytes.len); | ||
| const arraybuffer = env.createExternalArrayBuffer(buf, externalUint8Finalize, len_hint) catch |err| { | ||
| allocator.free(buf); | ||
| return err; | ||
| }; | ||
|
|
||
| _ = try env.adjustExternalMemory(@intCast(bytes.len)); | ||
| return try env.createTypedarray(.uint8, bytes.len, arraybuffer, 0); | ||
| } |
There was a problem hiding this comment.
To adhere to the repository style guide (Lines 51-55), this function should include assertions for its arguments and maintain a minimum assertion density of two per function. Additionally, the return value of adjustExternalMemory should be asserted rather than ignored (Line 51). The callback name in the createExternalArrayBuffer call should also be updated to createExternalUint8ArrayFinalize to match the renamed finalizer for better clarity (Line 246).
fn createExternalUint8Array(env: napi.Env, bytes: []const u8) !napi.Value {
std.debug.assert(bytes.len > 0);
std.debug.assert(bytes.len <= std.math.maxInt(i64));
const buf = try allocator.alloc(u8, bytes.len);
@memcpy(buf, bytes);
const len_hint: ?*anyopaque = @ptrFromInt(bytes.len);
const arraybuffer = env.createExternalArrayBuffer(buf, createExternalUint8ArrayFinalize, len_hint) catch |err| {
allocator.free(buf);
return err;
};
const adjusted = try env.adjustExternalMemory(@intCast(bytes.len));
std.debug.assert(adjusted >= @as(i64, @intCast(bytes.len)));
return try env.createTypedarray(.uint8, bytes.len, arraybuffer, 0);
}
References
- Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. (link)
- When a single function calls out to a helper function or callback, prefix the name of the helper function with the name of the calling function to show the call history. (link)
| fn externalUint8Finalize(env: napi.c.napi_env, data: ?*anyopaque, hint: ?*anyopaque) callconv(.c) void { | ||
| const len: usize = @intFromPtr(hint); | ||
| if (data) |d| { | ||
| const ptr: [*]u8 = @ptrCast(d); | ||
| allocator.free(ptr[0..len]); | ||
| } | ||
| var result: i64 = undefined; | ||
|
|
||
| _ = napi.status.check(napi.c.napi_adjust_external_memory(env, -@as(i64, @intCast(len)), &result)) catch return {}; | ||
| } |
There was a problem hiding this comment.
The finalizer should be renamed to createExternalUint8ArrayFinalize to clearly associate it with its caller (Line 246). It also requires assertions for all arguments (Line 51). Furthermore, architecture-specific usize should be avoided in favor of explicitly-sized types like u64 (Line 44), and the long line calling napi_adjust_external_memory should be wrapped to stay within the 100-column limit (Line 400). The error handling should also be more robust than a silent catch (Line 46, 160).
fn createExternalUint8ArrayFinalize(env: napi.c.napi_env, data: ?*anyopaque, hint: ?*anyopaque) callconv(.c) void {
std.debug.assert(env != null);
std.debug.assert(data != null);
std.debug.assert(hint != null);
const len: u64 = @intCast(@intFromPtr(hint));
if (data) |d| {
const ptr: [*]u8 = @ptrCast(d);
allocator.free(ptr[0..@intCast(len)]);
}
var result: i64 = undefined;
_ = napi.status.check(napi.c.napi_adjust_external_memory(
env,
-@as(i64, @intCast(len)),
&result,
)) catch {
std.debug.assert(false);
};
}
| const buf = try allocator.alloc(u8, bytes.len); | ||
| @memcpy(buf, bytes); |
There was a problem hiding this comment.
can use allocator.dupe as a shorthand
| return b.getValueBool(); | ||
| } | ||
|
|
||
| /// Wraps `bytes` as a JS `Uint8Array` backed by an *external* (native-heap) ArrayBuffer. |
There was a problem hiding this comment.
I'd change the comment here to mentioning duping, copying, etc. Current comment sounds like bytes is being captured.
|
|
||
| const len_hint: ?*anyopaque = @ptrFromInt(bytes.len); | ||
| const arraybuffer = env.createExternalArrayBuffer(buf, externalUint8Finalize, len_hint) catch |err| { | ||
| allocator.free(buf); |
There was a problem hiding this comment.
you should use an errdefer allocator.free(buf) above, and call env.createExternalArrayBuffer with try.
| var result: i64 = undefined; | ||
|
|
||
| _ = napi.status.check(napi.c.napi_adjust_external_memory(env, -@as(i64, @intCast(len)), &result)) catch return {}; |
There was a problem hiding this comment.
don't need result or napi.status.check since neither are intended to be used.
|
@wemeetagain thanks for the review! Though this is largely irrelevant now since the layer that handles creating of array buffers is moved to zapi, will open a PR there instead |
ported from ChainSafe/lodestar-z#356 External array buffers have their lifetimes managed by V8's garbage collector, but their backing memory is still managed by the native implementation. We need to call `adjustExternalMemory` to let V8 know about the native allocations; and we need a finalizer to cleanup such allocations (which we add in this PR, and use in lodestar-z) More details from that PR: > This is one of possible likely causes for increased GC pressure on experiments to swap out blst-ts for lodestar-z/bls, as observed on feat2 and feat3 deployments in [this PR](ChainSafe/lodestar#9342). > > With external array buffers, V8 is only aware of the pointer to the backing memory, instead of having to track both the pointer and the backing memory. This means that during marking phase the GC does not have to walk the backing memory to mark it as 'live' - the frequency of the GC firing off is still the same, but each cycle does less work. > > This of course comes with a tradeoff, we need a **finalizer** to let V8 know how much external memory is in native heap so that the GC tells the native impl to free the useless memory. > > Though, regardless of the effect, we should still probably do this anyway, since [napi-rs does the same](https://github.com/napi-rs/napi-rs/blob/159395b365c583a6642ad481edc5708d9f36a24b/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs#L175), and only defaults to V8 managed array buffers if it is disallowed (like in Electron).
* feat: support create_external_arraybuffer ported from ChainSafe/lodestar-z#356 External array buffers have their lifetimes managed by V8's garbage collector, but their backing memory is still managed by the native implementation. We need to call `adjustExternalMemory` to let V8 know about the native allocations; and we need a finalizer to cleanup such allocations (which we add in this PR, and use in lodestar-z) More details from that PR: > This is one of possible likely causes for increased GC pressure on experiments to swap out blst-ts for lodestar-z/bls, as observed on feat2 and feat3 deployments in [this PR](ChainSafe/lodestar#9342). > > With external array buffers, V8 is only aware of the pointer to the backing memory, instead of having to track both the pointer and the backing memory. This means that during marking phase the GC does not have to walk the backing memory to mark it as 'live' - the frequency of the GC firing off is still the same, but each cycle does less work. > > This of course comes with a tradeoff, we need a **finalizer** to let V8 know how much external memory is in native heap so that the GC tells the native impl to free the useless memory. > > Though, regardless of the effect, we should still probably do this anyway, since [napi-rs does the same](https://github.com/napi-rs/napi-rs/blob/159395b365c583a6642ad481edc5708d9f36a24b/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs#L175), and only defaults to V8 managed array buffers if it is disallowed (like in Electron). * add example and test
This is one of possible likely causes for increased GC pressure on experiments to swap out blst-ts for lodestar-z/bls, as observed on feat2 and feat3 deployments in this PR.
With external array buffers, V8 is only aware of the pointer to the backing memory, instead of having to track both the pointer and the backing memory. This means that during marking phase the GC does not have to walk the backing memory to mark it as 'live' - the frequency of the GC firing off is still the same, but each cycle does less work.
This of course comes with a tradeoff, we need a finalizer to let V8 know how much external memory is in native heap so that the GC tells the native impl to free the useless memory.
Though, regardless of the effect, we should still probably do this anyway, since napi-rs does the same, and only defaults to V8 managed array buffers if it is disallowed (like in Electron).