refactor(bridge/core): daily performance optimization audit#159
Conversation
- Optimized `bindMap` using `v.MapRange()` and `strconv` to reduce interface boxing and allocations. - Optimized `bindSlice` with a fast path for `[]byte`, converting to `Uint8Array` safely using copied arrays. - Introduced `ToUint8Array` in `arraybuffer.go` with safe fallback to `ArrayBuffer`. - Improved cache locality in `EventLoop` struct by grouping `running` and `autoStop` with their `mu` sync.Mutex. - Ensured thread safety in `CreatePromise` using `sync.Once` to protect `wg.Done()` from multiple invocations. - Added appropriate `// @optimized` comments. 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 adds safe Go-to-JavaScript byte array conversion, optimizes map iteration and key formatting in the JS bridge reflection layer, and hardens promise settlement in the EventLoop to prevent duplicate callback executions and memory leaks. ChangesJS bridge and EventLoop improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bridge/core/reflection.go (1)
381-389: ⚡ Quick winThis optimization is intentional and currently has no impact on the codebase.
The recent change from
fmt.Sprint(key.Interface())tostrconvfor numeric keys was an intentional performance optimization (commit 230d8d1). However, there are no named numeric types withString()methods in the codebase, so no existing serialization behavior has changed. Thedefaultcase still usesfmt.Sprint(key.Interface())as a fallback.If named numeric types with custom
String()methods are added in the future and used as map keys, they would lose their custom string output. The suggested fix is valid defensive programming but addresses a hypothetical rather than an actual problem. If this pattern becomes a concern, it could be addressed then.🤖 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 381 - 389, The numeric key branches (switch on key.Kind()) currently format numbers with strconv.FormatInt/FormatUint which will bypass any custom String() implementations on named numeric types; update the numeric cases in the switch that handle reflect.Int*/reflect.Uint* to first detect if the key's type implements fmt.Stringer (use reflect.TypeOf((*fmt.Stringer)(nil)).Elem() and key.Type().Implements(...)) and if so set keyStr = fmt.Sprint(key.Interface()), otherwise fall back to strconv.FormatInt(key.Int(), 10) or strconv.FormatUint(key.Uint(), 10); keep the existing reflect.String case and the default fmt.Sprint fallback.
🤖 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/reflection.go`:
- Around line 381-389: The numeric key branches (switch on key.Kind()) currently
format numbers with strconv.FormatInt/FormatUint which will bypass any custom
String() implementations on named numeric types; update the numeric cases in the
switch that handle reflect.Int*/reflect.Uint* to first detect if the key's type
implements fmt.Stringer (use reflect.TypeOf((*fmt.Stringer)(nil)).Elem() and
key.Type().Implements(...)) and if so set keyStr = fmt.Sprint(key.Interface()),
otherwise fall back to strconv.FormatInt(key.Int(), 10) or
strconv.FormatUint(key.Uint(), 10); keep the existing reflect.String case and
the default fmt.Sprint fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44aad6c1-6a55-4553-ae9e-ba89d38f7ea4
📒 Files selected for processing (3)
bridge/core/arraybuffer.gobridge/core/reflection.goeventloop/eventloop.go
This pull request executes the daily performance and optimization audit for
bridge/coreandeventloop/eventloop.go.Changes Made:
bridge/core/reflection.go):v.MapKeys()tov.MapRange()inbindMapto avoid intermediate slice allocations.fmt.Sprint(key.Interface())with aswitchchecking numeric kinds and usingstrconv.FormatInt/Uint. This prevents boxing numbers intointerface{}.[]byteFast Path (bridge/core/reflection.go&arraybuffer.go):bindSliceto securely handle[]bytevalues. If addressable,v.Bytes()is used; otherwise, it falls back gracefully.ToUint8Arrayto convert Go byte slices into JavaScriptUint8Arrayinstances with safe fallbacks if the global doesn't exist.Uint8Arrayto avert JavaScript from mutating the host memory block.eventloop/eventloop.go):EventLoopstruct to couple therunningandautoStopboolean properties directly beside their protectingmuMutex, optimizing cache line efficiency.eventloop/eventloop.go):sync.OnceinCreatePromisecallbacks to reliably ensureel.wg.Done()is fired precisely once, forestallingsync.WaitGroupunderflow panics if JavaScript invokesresolve/rejectrepetitively.All features run flawlessly per existing unit, e2e, and integration tests. Evaluated through
make test&go test -v ./....PR created automatically by Jules for task 16463292254535626834 started by @repyh
Summary by CodeRabbit
Bug Fixes
Performance Improvements