perf(bridge/core): apply daily incremental optimizations and safety guardrails#156
perf(bridge/core): apply daily incremental optimizations and safety guardrails#156repyh wants to merge 1 commit into
Conversation
…uardrails - **eventloop**: Reordered `EventLoop` struct for better cache locality and added `sync.Once` wrapper around Promise resolution loop scheduling to prevent multiple `wg.Done()` calls. - **arraybuffer**: Enforced memory isolation in `ToArrayBuffer` by copying the byte slice before exposing it to the JS runtime to prevent memory mutation. Introduced strict fallback checks in `MapSharedBuffer` and a new `ToUint8Array` utility. - **reflection**: Implemented `MapRange` in `bindMap` to avoid `MapKeys` slice allocations and used `strconv` functions to bypass interface boxing string conversion for numeric keys. Added a fast-path in `bindSlice` to securely process `[]byte` mapping. 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 improves type conversion safety, optimizes reflection bindings, and fixes event loop promise synchronization. Array buffer operations now explicitly copy input bytes and defensively handle Uint8Array availability. Reflection binding gains a fast path for byte slices and more efficient map key formatting. Event loop promise settlement is guarded by sync.Once to ensure single execution. ChangesType Conversion and Reflection Optimizations
Event Loop Promise Synchronization
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bridge/core/arraybuffer.go (1)
50-63: ⚡ Quick winUpdate
MapSharedBufferdocs to reflect ArrayBuffer fallback.Behavior now conditionally exposes
ArrayBuffer, but Line 41-42 still says it is exposed asUint8Array. Please align the comment with runtime behavior.📝 Suggested doc update
-// as a Uint8Array in JavaScript. This is commonly used for inter-worker -// communication and zero-copy data sharing. +// as a Uint8Array when available. In restricted runtimes 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 50 - 63, Update the documentation/comment for MapSharedBuffer to match the implementation: state that MapSharedBuffer tries to construct a Uint8Array view (using ctor := vm.Get("Uint8Array") and vm.New(ctor.ToObject(vm), buf)) and, if that fails, falls back to exposing the raw ArrayBuffer (view = buf) under the given name; modify the comment near the MapSharedBuffer function to clearly mention the conditional behavior and fallback to ArrayBuffer instead of always claiming it is exposed as Uint8Array.
🤖 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.
Inline comments:
In `@eventloop/eventloop.go`:
- Around line 126-146: CreatePromise currently calls el.wg.Add(1) after Start's
auto-stop goroutine may have already observed zero, violating sync.WaitGroup
semantics and causing a panic; fix by ensuring the promise's liveness is
registered before the auto-stop waiter can return: either (1) guard
CreatePromise with the running state (check the Start/Stop-running flag in Start
and refuse/fail fast in CreatePromise), or (2) move the el.wg.Add(1)
registration to a point that happens before the auto-stop goroutine is started
(or start the auto-stop goroutine only after an initial reservation), or (3)
disable auto-stop when asynchronous promises are expected; reference
CreatePromise, Start, el.wg.Add(1), and the auto-stop goroutine to locate the
changes.
---
Nitpick comments:
In `@bridge/core/arraybuffer.go`:
- Around line 50-63: Update the documentation/comment for MapSharedBuffer to
match the implementation: state that MapSharedBuffer tries to construct a
Uint8Array view (using ctor := vm.Get("Uint8Array") and
vm.New(ctor.ToObject(vm), buf)) and, if that fails, falls back to exposing the
raw ArrayBuffer (view = buf) under the given name; modify the comment near the
MapSharedBuffer function to clearly mention the conditional behavior and
fallback to ArrayBuffer instead of always claiming it is exposed as Uint8Array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c81f3529-27ff-4698-95dc-e110d0eda1d7
📒 Files selected for processing (3)
bridge/core/arraybuffer.gobridge/core/reflection.goeventloop/eventloop.go
| // Keep the loop alive until the promise is settled | ||
| el.wg.Add(1) | ||
|
|
||
| // @optimized: Use sync.Once to ensure loop scheduling and wg.Done() are called exactly once per promise. | ||
| var once sync.Once | ||
|
|
||
| resolve = func(v interface{}) { | ||
| el.RunOnLoop(func() { | ||
| _ = res(v) | ||
| el.wg.Done() | ||
| once.Do(func() { | ||
| el.RunOnLoop(func() { | ||
| _ = res(v) | ||
| el.wg.Done() | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| reject = func(v interface{}) { | ||
| el.RunOnLoop(func() { | ||
| _ = rej(v) | ||
| el.wg.Done() | ||
| once.Do(func() { | ||
| el.RunOnLoop(func() { | ||
| _ = rej(v) | ||
| el.wg.Done() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the Go sync.WaitGroupdocumentation, is it safe to callAdd(1)after another goroutine has already calledWait() and observed the counter as zero?
💡 Result:
No, it is not safe to call Add(1) after a WaitGroup's counter has reached zero and a Wait call has returned [1][2]. According to the official Go documentation for sync.WaitGroup, calls with a positive delta that occur when the counter is zero must happen before a Wait call [1][2]. If you intend to reuse a WaitGroup to wait for several independent sets of events, any new Add calls must occur only after all previous Wait calls have returned [1][2]. Attempting to call Add after Wait has already unblocked (because the counter reached zero) is considered a misuse of the WaitGroup and can lead to panics or race conditions [1][3]. The implementation of Add includes explicit checks to detect such misuse, and it will panic if it detects that Add is being called concurrently with Wait or after the counter has been reset by a completed Wait operation [1].
Citations:
- 1: https://github.com/golang/go/blob/go1.20.12/src/sync/waitgroup.go
- 2: https://pkg.go.dev/sync
- 3: https://medium.com/@debdut.saha.1/what-i-found-when-i-finally-read-the-sync-waitgroup-source-code-personal-relatable-17477d60a8fa
🏁 Script executed:
cd eventloop 2>/dev/null || cd . && find . -name "eventloop.go" -type fRepository: repyh/typego
Length of output: 71
🏁 Script executed:
wc -l ./eventloop.go && echo "---" && cat -n ./eventloop.goRepository: repyh/typego
Length of output: 102
🏁 Script executed:
find . -name "eventloop.go" -type fRepository: repyh/typego
Length of output: 81
🏁 Script executed:
wc -l ./eventloop/eventloop.go && echo "---" && cat -n ./eventloop/eventloop.goRepository: repyh/typego
Length of output: 3779
CreatePromise violates Go sync.WaitGroup semantics and will panic.
If Start() is called with autoStop enabled (the default), the auto-stop goroutine at line 60–63 will immediately observe the WaitGroup counter as zero and call Wait(), returning instantly. Any subsequent call to CreatePromise() will then call Add(1) at line 127 on a WaitGroup that has already returned from Wait() with counter zero. According to Go's sync.WaitGroup documentation, this is explicitly forbidden and will panic at runtime.
To fix this, either:
- Prevent
CreatePromise()from being called afterStart()(e.g., checkrunningflag and fail fast), or - Register the promise's liveness before the auto-stop waiter can observe zero (e.g., call
Add(1)before launching the auto-stop goroutine), or - Disable auto-stop when promises may be created asynchronously.
🤖 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 `@eventloop/eventloop.go` around lines 126 - 146, CreatePromise currently calls
el.wg.Add(1) after Start's auto-stop goroutine may have already observed zero,
violating sync.WaitGroup semantics and causing a panic; fix by ensuring the
promise's liveness is registered before the auto-stop waiter can return: either
(1) guard CreatePromise with the running state (check the Start/Stop-running
flag in Start and refuse/fail fast in CreatePromise), or (2) move the
el.wg.Add(1) registration to a point that happens before the auto-stop goroutine
is started (or start the auto-stop goroutine only after an initial reservation),
or (3) disable auto-stop when asynchronous promises are expected; reference
CreatePromise, Start, el.wg.Add(1), and the auto-stop goroutine to locate the
changes.
1 participant
Daily performance audit for the
bridge/coreandeventloopmodule. Focusing on cache locality, strict type formatting, and slice isolation for shared memory bindings between Go and JS. Fixes multiple safety edge cases while providing direct performance uplifts.PR created automatically by Jules for task 655556913855000679 started by @repyh
Summary by CodeRabbit
Bug Fixes
Performance