Refactor: Daily Core and Eventloop Optimizations#147
Conversation
- Upgraded bindMap to use MapRange and explicit strconv for integer keys - Enhanced bindSlice to route byte slices via ToUint8Array safely - Secured CreatePromise with sync.Once to prevent wg.Done panics - Reordered EventLoop struct fields to improve cache locality 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 three coordinated improvements: a new ChangesGo-to-JS Bridging and Promise Safety Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (2)
bridge/core/arraybuffer.go (1)
17-33: 💤 Low valueDocument and consider the fallback type divergence.
When
Uint8Arrayis missing or its construction fails, the helper silently returns anArrayBufferinstead of a typed view. Callers (e.g.,bindSlice's[]bytefast-path) receive a value with a different JS surface (no indexed element access, no.lengthin element units, no typed iteration). It would be worth either documenting this fallback in the function-level comment so callers are aware, or returning anerror/boolso the caller can decide. In a normal Sobek runtimeUint8Arrayis always present, so this is a low-likelihood path, but worth noting.📝 Proposed doc clarification
// ToUint8Array converts a Go byte slice to a JavaScript Uint8Array. -// `@optimized`: Safely instantiate a Uint8Array view over an ArrayBuffer. +// `@optimized`: Safely instantiate a Uint8Array view over an ArrayBuffer. +// +// If the global Uint8Array constructor is unavailable or view construction +// fails, the underlying ArrayBuffer is returned as a fallback. Callers that +// rely on typed-array semantics (indexed access, .length, etc.) should be +// prepared to handle this case. func ToUint8Array(vm *sobek.Runtime, data []byte) sobek.Value {🤖 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 17 - 33, The ToUint8Array function currently returns an ArrayBuffer when Uint8Array is absent or construction fails, which changes the JS surface for callers; update ToUint8Array to either (A) document this fallback in the function comment (explicitly state that it returns an ArrayBuffer when a Uint8Array cannot be created and what behavioral differences callers should expect), or (B) change the signature to return an indicator (e.g., (sobek.Value, bool) or (sobek.Value, error)) so callers like bindSlice can detect the divergence and handle it; if you choose (B), update callers (e.g., bindSlice) to check the boolean/error and implement an explicit fallback path or surface an appropriate error to the caller.bridge/core/reflection.go (1)
343-355: ⚡ Quick winAvoid the redundant second copy in the unaddressable-array branch.
In the
elsebranch (lines 347-350)bytesis already a freshly allocated, independent slice produced byreflect.Copy. The unconditional copy at lines 353-354 then allocates and copies the same N bytes again, doubling allocation/work for byte arrays. Consolidating into a single allocation per branch keeps the slice-/addressable-array path correct (where the copy is needed becausev.Bytes()shares memory) without paying twice for arrays.♻️ Proposed consolidation
// `@optimized`: Directly convert []byte to Uint8Array avoiding generic interface conversion. if v.Type().Elem() == reflect.TypeOf(byte(0)) { - var bytes []byte + var copied []byte if v.CanAddr() || v.Kind() == reflect.Slice { - bytes = v.Bytes() + // v.Bytes() returns a view over backing memory; copy to prevent + // shared-memory mutation between Go and JS. + src := v.Bytes() + copied = make([]byte, len(src)) + copy(copied, src) } else { - // Unaddressable array, must copy to slice - bytes = make([]byte, v.Len()) - reflect.Copy(reflect.ValueOf(bytes), v) + // Unaddressable array, must copy to slice (already independent). + copied = make([]byte, v.Len()) + reflect.Copy(reflect.ValueOf(copied), v) } - - // Copy to prevent shared memory mutation - copied := make([]byte, len(bytes)) - copy(copied, bytes) return ToUint8Array(vm, copied), nil }🤖 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/reflection.go` around lines 343 - 355, The current code always allocates a second copy (`copied`) even when the `else` branch already produced a fresh slice via `reflect.Copy`; change the logic so that only the path that calls `v.Bytes()` (the addressable/slice branch) performs the extra allocation+copy, while the unaddressable-array branch returns the slice produced by `reflect.Copy` directly. Concretely, in the block around `v.Bytes()` and `reflect.Copy(...)` use `bytes := v.Bytes()` then allocate `copied := make([]byte, len(bytes)); copy(copied, bytes); return ToUint8Array(vm, copied)` for the addressable/slice case, and in the `else` branch keep `bytes := make([]byte, v.Len()); reflect.Copy(reflect.ValueOf(bytes), v); return ToUint8Array(vm, bytes)` so no redundant second allocation is performed; keep references to `v`, `bytes`, `copied`, `reflect.Copy`, and `ToUint8Array` to find the code.
🤖 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 130-148: The promise settlement handlers (resolve/reject) enqueue
unconditionally via el.RunOnLoop causing an extra el.wg.Add in RunOnLoop and a
potential leaked wait-group if the loop is stopped; modify resolve and reject in
CreatePromise to check whether the loop is running (e.g., using the same
stopped/state flag that Stop() sets) and if the loop is stopped call res/rej
synchronously and el.wg.Done() without calling el.RunOnLoop, otherwise keep the
current RunOnLoop path; reference CreatePromise's resolve/reject, el.RunOnLoop,
el.wg.Add/el.wg.Done, and Stop/Shutdown when making the change.
---
Nitpick comments:
In `@bridge/core/arraybuffer.go`:
- Around line 17-33: The ToUint8Array function currently returns an ArrayBuffer
when Uint8Array is absent or construction fails, which changes the JS surface
for callers; update ToUint8Array to either (A) document this fallback in the
function comment (explicitly state that it returns an ArrayBuffer when a
Uint8Array cannot be created and what behavioral differences callers should
expect), or (B) change the signature to return an indicator (e.g., (sobek.Value,
bool) or (sobek.Value, error)) so callers like bindSlice can detect the
divergence and handle it; if you choose (B), update callers (e.g., bindSlice) to
check the boolean/error and implement an explicit fallback path or surface an
appropriate error to the caller.
In `@bridge/core/reflection.go`:
- Around line 343-355: The current code always allocates a second copy
(`copied`) even when the `else` branch already produced a fresh slice via
`reflect.Copy`; change the logic so that only the path that calls `v.Bytes()`
(the addressable/slice branch) performs the extra allocation+copy, while the
unaddressable-array branch returns the slice produced by `reflect.Copy`
directly. Concretely, in the block around `v.Bytes()` and `reflect.Copy(...)`
use `bytes := v.Bytes()` then allocate `copied := make([]byte, len(bytes));
copy(copied, bytes); return ToUint8Array(vm, copied)` for the addressable/slice
case, and in the `else` branch keep `bytes := make([]byte, v.Len());
reflect.Copy(reflect.ValueOf(bytes), v); return ToUint8Array(vm, bytes)` so no
redundant second allocation is performed; keep references to `v`, `bytes`,
`copied`, `reflect.Copy`, and `ToUint8Array` to find the code.
🪄 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: d76401c2-b21b-4592-b736-0fbb58cbab07
📒 Files selected for processing (3)
bridge/core/arraybuffer.gobridge/core/reflection.goeventloop/eventloop.go
| // @optimized: sync.Once ensures wg.Done() is called exactly once to prevent panics | ||
| 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
🏁 Script executed:
# Find and examine eventloop.go
find . -name "eventloop.go" -type fRepository: repyh/typego
Length of output: 81
🏁 Script executed:
# Get file size to determine how to read it
wc -l eventloop/eventloop.go 2>/dev/null || find . -name "eventloop.go" -exec wc -l {} \;Repository: repyh/typego
Length of output: 83
🏁 Script executed:
# Read the CreatePromise method around lines 130-148
find . -name "eventloop.go" -exec sed -n '100,160p' {} \;Repository: repyh/typego
Length of output: 982
🏁 Script executed:
# Find RunOnLoop implementation to verify wait-group behavior
find . -name "eventloop.go" -exec grep -n "func.*RunOnLoop" {} +Repository: repyh/typego
Length of output: 102
🏁 Script executed:
# Find Stop implementation to understand shutdown behavior
find . -name "eventloop.go" -exec grep -n "func.*Stop" {} +Repository: repyh/typego
Length of output: 140
🏁 Script executed:
# Look at the full CreatePromise method to understand context availability
find . -name "eventloop.go" -exec grep -n "func.*CreatePromise" {} +Repository: repyh/typego
Length of output: 176
🏁 Script executed:
# Read RunOnLoop implementation (lines 79-83)
sed -n '79,83p' eventloop/eventloop.goRepository: repyh/typego
Length of output: 134
🏁 Script executed:
# Read Stop implementation (lines 84-95)
sed -n '84,95p' eventloop/eventloop.goRepository: repyh/typego
Length of output: 257
🏁 Script executed:
# Read the beginning of the file to understand EventLoop struct
sed -n '1,50p' eventloop/eventloop.goRepository: repyh/typego
Length of output: 1048
🏁 Script executed:
# Verify the exact line numbers and current code once more
sed -n '124,148p' eventloop/eventloop.goRepository: repyh/typego
Length of output: 625
Handle promise settlement after shutdown.
Lines 135 and 144 enqueue unconditionally, creating a wait-group accounting bug: CreatePromise calls el.wg.Add(1) (line 130) and RunOnLoop calls el.wg.Add(1) again (line 80), but the callback only calls el.wg.Done() once. If resolve/reject is invoked after Stop(), RunOnLoop queues a job that will never execute, leaving the wait-group unbalanced and causing Shutdown() to hang indefinitely.
Suggested fix
// `@optimized`: sync.Once ensures wg.Done() is called exactly once to prevent panics
var once sync.Once
+ settle := func(fn func(interface{}) error, v interface{}) {
+ el.wg.Add(1) // queued job bookkeeping
+ job := func() {
+ defer el.wg.Done() // queued job bookkeeping
+ defer el.wg.Done() // promise keepalive
+ _ = fn(v)
+ }
+
+ select {
+ case <-el.ctx.Done():
+ el.wg.Done() // undo queued job bookkeeping
+ el.wg.Done() // release promise keepalive
+ case el.jobQueue <- job:
+ }
+ }
resolve = func(v interface{}) {
once.Do(func() {
- el.RunOnLoop(func() {
- _ = res(v)
- el.wg.Done()
- })
+ settle(res, v)
})
}
reject = func(v interface{}) {
once.Do(func() {
- el.RunOnLoop(func() {
- _ = rej(v)
- el.wg.Done()
- })
+ settle(rej, v)
})
}🤖 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 130 - 148, The promise settlement
handlers (resolve/reject) enqueue unconditionally via el.RunOnLoop causing an
extra el.wg.Add in RunOnLoop and a potential leaked wait-group if the loop is
stopped; modify resolve and reject in CreatePromise to check whether the loop is
running (e.g., using the same stopped/state flag that Stop() sets) and if the
loop is stopped call res/rej synchronously and el.wg.Done() without calling
el.RunOnLoop, otherwise keep the current RunOnLoop path; reference
CreatePromise's resolve/reject, el.RunOnLoop, el.wg.Add/el.wg.Done, and
Stop/Shutdown when making the change.
1 participant
Daily optimization patch addressing interface boxing, slice bindings, cache line efficiency, and wait group panic prevention.
PR created automatically by Jules for task 12270570119718624561 started by @repyh
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance