perf: optimize reflection and eventloop structures#165
Conversation
- Implemented `MapRange()` for map iteration in `bindMap` to avoid key slice allocations. - Replaced `fmt.Sprint` with `strconv` formatters in `bindMap` to prevent interface boxing for numeric keys. - Added a fast path for `[]byte` in `bindSlice` that safely creates a `Uint8Array` wrapper instead of iterating, drastically improving speed while maintaining safety. - Rearranged fields in `EventLoop` struct for better cache locality. - Added `sync.Once` in `CreatePromise` to prevent waitgroup panics. 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. |
📝 WalkthroughWalkthroughTwo separate improvements are made across the codebase: reflection binding is optimized with a fast path for byte slices (Uint8Array conversion) and efficient map key stringification using strconv, while EventLoop promise settlement is made concurrency-safe using sync.Once to prevent double-wg-done panics. ChangesReflection Binding Optimizations
EventLoop Promise Safety and Cache Locality
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
🤖 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 `@bridge/core/reflection.go`:
- Around line 398-406: The switch on key.Kind() forces strconv formatting and
loses custom fmt.Stringer semantics for named numeric key types; update the
logic around the switch where key is inspected so that before falling into the
numeric kinds you detect if the key's type implements fmt.Stringer (e.g., via a
package-level var for reflect.TypeOf((*fmt.Stringer)(nil)).Elem() and
key.Type().Implements(...) or by key.CanInterface()/key.Interface() type-assert
to fmt.Stringer) and if so set keyStr = fmt.Sprint(key.Interface()); otherwise
keep the existing strconv.FormatInt/FormatUint branches for raw numeric kinds.
🪄 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: c1f754c9-7ee6-45dc-abe8-62a0f6ed19f3
📒 Files selected for processing (2)
bridge/core/reflection.goeventloop/eventloop.go
| switch key.Kind() { | ||
| case reflect.String: | ||
| keyStr = key.String() | ||
| } else { | ||
| case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: | ||
| keyStr = strconv.FormatInt(key.Int(), 10) | ||
| case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: | ||
| keyStr = strconv.FormatUint(key.Uint(), 10) | ||
| default: | ||
| keyStr = fmt.Sprint(key.Interface()) |
There was a problem hiding this comment.
Preserve fmt.Stringer semantics for named numeric key types.
This fast path changes output for custom numeric key types (e.g., types with String()), because it forces strconv by kind and skips fmt.Sprint. That can break existing JS key names.
Proposed fix
- switch key.Kind() {
+ keyType := key.Type()
+ switch key.Kind() {
case reflect.String:
keyStr = key.String()
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
- keyStr = strconv.FormatInt(key.Int(), 10)
+ if keyType.PkgPath() == "" { // builtin numeric types
+ keyStr = strconv.FormatInt(key.Int(), 10)
+ } else {
+ keyStr = fmt.Sprint(key.Interface())
+ }
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
- keyStr = strconv.FormatUint(key.Uint(), 10)
+ if keyType.PkgPath() == "" { // builtin numeric types
+ keyStr = strconv.FormatUint(key.Uint(), 10)
+ } else {
+ keyStr = fmt.Sprint(key.Interface())
+ }
default:
keyStr = fmt.Sprint(key.Interface())
}🤖 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 398 - 406, The switch on key.Kind()
forces strconv formatting and loses custom fmt.Stringer semantics for named
numeric key types; update the logic around the switch where key is inspected so
that before falling into the numeric kinds you detect if the key's type
implements fmt.Stringer (e.g., via a package-level var for
reflect.TypeOf((*fmt.Stringer)(nil)).Elem() and key.Type().Implements(...) or by
key.CanInterface()/key.Interface() type-assert to fmt.Stringer) and if so set
keyStr = fmt.Sprint(key.Interface()); otherwise keep the existing
strconv.FormatInt/FormatUint branches for raw numeric kinds.
1 participant
Perf improvements for
bridge/coreandeventloop.Daily Scorecard: 9/10
The codebase is now significantly more optimized, eliminating major overhead in reflection maps/slices and preventing waitgroup panics.
Micro-audits:
context.Background()initially withcontext.WithTimeout/WithCanceland correctly tracked. No instances ofcontext.TODO().sync.MutexinServerandEventLoopare appropriate due to low contention/short scopes.sync.RWMutexis appropriately used in AsyncMutex.PR created automatically by Jules for task 4337362432450148805 started by @repyh
Summary by CodeRabbit
Release Notes
Bug Fixes
Performance Improvements