perf(core): daily incremental optimizations and safety harding#164
perf(core): daily incremental optimizations and safety harding#164repyh wants to merge 1 commit into
Conversation
- Adds `[]byte` fast path to `bindSlice` with explicit slice copying for memory isolation. - Converts `bindMap` to use `MapRange()` and `strconv` primitives, avoiding interface boxing and slice allocation. - Rearranges `EventLoop` struct fields to maximize cache locality and group variables under their protecting mutex. - Implements `sync.Once` in `CreatePromise` to prevent wait group panics from double-settling. - Consolidates console logging to prevent double-locking `os.Stdout`. - Implements safe fallback for `Uint8Array` constructor resolution in `MapSharedBuffer`. Co-authored-by: repyh <63894915+repyh@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR introduces reliability and performance improvements across the Go-to-JavaScript bridge layer and event loop. Changes add fallback handling for missing ArrayBuffer constructors, optimize byte slice and map binding in reflection, fix console error output formatting, and ensure promise callbacks execute safely only once via synchronization primitives. ChangesBridge and EventLoop Enhancements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bridge/core/arraybuffer.go (1)
34-45: ⚡ Quick winDocument the new
ArrayBufferfallback behavior inMapSharedBuffer.The implementation now conditionally exposes raw
ArrayBuffer, but the function docs still state it is exposed asUint8Array. Please update the docstring to reflect both outcomes.Proposed doc update
// The buffer is registered as a global variable with the given name, accessible -// as a Uint8Array in JavaScript. This is commonly used for inter-worker +// as a Uint8Array in JavaScript when available. In environments where +// Uint8Array is unavailable, it falls back to exposing the ArrayBuffer directly. +// This is commonly used for inter-worker // communication and zero-copy data sharing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bridge/core/arraybuffer.go` around lines 34 - 45, The MapSharedBuffer docstring still says the buffer is exposed as a Uint8Array but the implementation (in MapSharedBuffer) falls back to exposing a raw ArrayBuffer when Uint8Array is unavailable; update the function comment to describe both outcomes: when the global Uint8Array exists MapSharedBuffer exposes a Uint8Array view (using ctor/typedArray), and when it does not it exposes the raw ArrayBuffer (vm.ToValue(buf)), including the conditions checked (ctor == nil || sobek.IsNull(ctor) || sobek.IsUndefined(ctor)) and the rationale for the graceful fallback so callers know to handle either type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@bridge/core/arraybuffer.go`:
- Around line 34-45: The MapSharedBuffer docstring still says the buffer is
exposed as a Uint8Array but the implementation (in MapSharedBuffer) falls back
to exposing a raw ArrayBuffer when Uint8Array is unavailable; update the
function comment to describe both outcomes: when the global Uint8Array exists
MapSharedBuffer exposes a Uint8Array view (using ctor/typedArray), and when it
does not it exposes the raw ArrayBuffer (vm.ToValue(buf)), including the
conditions checked (ctor == nil || sobek.IsNull(ctor) ||
sobek.IsUndefined(ctor)) and the rationale for the graceful fallback so callers
know to handle either type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b548d0df-bca6-4af3-b617-952ce5234fed
📒 Files selected for processing (4)
bridge/core/arraybuffer.gobridge/core/console.gobridge/core/reflection.goeventloop/eventloop.go
1 participant
This daily optimization patch addresses multiple areas in the
bridge/coreandeventlooppackages to improve execution speed, minimize memory allocations, and enhance runtime safety.Key Changes:
bindSlice&bindMap): Eliminated interface boxing and allocation overhead for standard numeric/byte slice mapping into Sobek.EventLoopto ensure hot path data avoids false sharing and operates within shared cache lines alongside their lock.Resolve/Rejectcalls on Sobek promises using Gosync.Onceprimitives. Prevented potential fatal errors when operating in JS environments lacking globalUint8Arrayvariables.Bench Recommendation:
To verify the impact of the reflection path improvements, run:
go test -bench=. ./bridge/corePR created automatically by Jules for task 8474244443530169332 started by @repyh
Summary by CodeRabbit
Bug Fixes
Refactor