perf(bridge): optimize map and array binding to eliminate allocation overhead#152
perf(bridge): optimize map and array binding to eliminate allocation overhead#152repyh wants to merge 2 commits into
Conversation
- Added `ToUint8Array` to `arraybuffer.go` for memory-safe JS mapping. - Restored fast-path for `[]byte` in `bindSlice` leveraging `ToUint8Array`. - Migrated `bindMap` from `MapKeys()` to `MapRange()` iterator to prevent slice allocation. - Bypassed heavy `fmt.Sprint` in map string conversions with direct `strconv` formatting for integers. 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. |
📝 WalkthroughWalkthroughAdds ToUint8Array and fast-path []byte binding; improves map key iteration/formatting; tracks inspected external modules to avoid emitting uninspected external imports in shim generation; pins an example Gin dependency to v1.11.0. ChangesByte Array and Map Binding Optimization
CLI shim import inspection
Example dependency pin
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bridge/core/reflection.go (1)
379-387: ⚡ Quick winConsider fmt.Stringer support for named numeric map keys if needed.
The current implementation uses
reflect.Kind()to optimize integer key formatting viastrconv, bypassingfmt.Stringerfor named numeric types liketype Status int. If customString()methods are expected on such keys, add a type assertion before the kind-based switch. However, this optimization was intentional for performance; verify whetherfmt.Stringersupport for numeric keys is actually required before changing it.🤖 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 379 - 387, The switch on key.Kind() that sets keyStr (in the code handling map key formatting) bypasses fmt.Stringer for named numeric types (e.g., type Status int); modify the logic in the map-key formatting path to first type-assert the reflect.Value's underlying Interface() to fmt.Stringer and, if successful, use its String() result for keyStr, otherwise fall back to the existing kind-based strconv/ fmt.Sprint branches (ensure you reference the same variables/keyStr and the switch on key.Kind()).
🤖 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 379-387: The switch on key.Kind() that sets keyStr (in the code
handling map key formatting) bypasses fmt.Stringer for named numeric types
(e.g., type Status int); modify the logic in the map-key formatting path to
first type-assert the reflect.Value's underlying Interface() to fmt.Stringer
and, if successful, use its String() result for keyStr, otherwise fall back to
the existing kind-based strconv/ fmt.Sprint branches (ensure you reference the
same variables/keyStr and the switch on key.Kind()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48f920a9-fc0f-4e6b-ba1c-d84990f79028
📒 Files selected for processing (2)
bridge/core/arraybuffer.gobridge/core/reflection.go
… and fix CI import errors - Added `ToUint8Array` to `arraybuffer.go` for memory-safe JS mapping. - Restored fast-path for `[]byte` in `bindSlice` leveraging `ToUint8Array`. - Migrated `bindMap` from `MapKeys()` to `MapRange()` iterator to prevent slice allocation. - Bypassed heavy `fmt.Sprint` in map string conversions with direct `strconv` formatting for integers. - Pinned `gin-gonic/gin` to `v1.11.0` in `examples/projects/link-shortener/typego.modules.json` to fix a Go 1.25.0 version incompatibility that breaks the `examples/projects/link-shortener` checks. - Prevented `imported and not used` Go compiler errors in `pkg/cli/cmd/build.go` and `pkg/cli/cmd/run.go` by verifying modules properly before appending them to the generated shim import block. Co-authored-by: repyh <63894915+repyh@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cli/cmd/build.go (1)
105-110: ⚡ Quick winClarify the comment to accurately describe the skip condition.
The comment states "or external modules that successfully inspected," but the code actually skips external modules that were NOT successfully inspected. Consider revising to:
// Skip external modules (containing '.') that failed inspection. // This prevents "imported and not used" errors for modules that couldn't be inspected.🤖 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 `@pkg/cli/cmd/build.go` around lines 105 - 110, Update the misleading comment above the skip condition in build.go to accurately describe the logic: it skips external modules (identified by '.' in cleanImp) that were NOT successfully inspected according to inspectedModules, to avoid "imported and not used" errors; reference the inspectedModules map and the cleanImp variable and the if !inspectedModules[cleanImp] && strings.Contains(cleanImp, ".") check when updating the comment.pkg/cli/cmd/run.go (1)
130-132: 💤 Low valueConsider adding an explanatory comment.
This filtering logic is identical to
build.gobut lacks explanatory comments. Adding a brief comment would improve maintainability:// Skip external modules (containing '.') that failed inspection to prevent "imported and not used" errors. if !inspectedModules[cleanImp] && strings.Contains(cleanImp, ".") { continue }🤖 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 `@pkg/cli/cmd/run.go` around lines 130 - 132, Add a brief explanatory comment immediately above the if that checks inspectedModules and strings.Contains(cleanImp, ".") in run.go: explain that the branch skips external (dot-qualified) imports that weren't inspected to avoid "imported and not used" errors, matching the identical logic in build.go; keep the existing condition (if !inspectedModules[cleanImp] && strings.Contains(cleanImp, ".") { continue }) unchanged and only add the one-line comment for maintainability and clarity.
🤖 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 `@pkg/cli/cmd/build.go`:
- Around line 105-110: Update the misleading comment above the skip condition in
build.go to accurately describe the logic: it skips external modules (identified
by '.' in cleanImp) that were NOT successfully inspected according to
inspectedModules, to avoid "imported and not used" errors; reference the
inspectedModules map and the cleanImp variable and the if
!inspectedModules[cleanImp] && strings.Contains(cleanImp, ".") check when
updating the comment.
In `@pkg/cli/cmd/run.go`:
- Around line 130-132: Add a brief explanatory comment immediately above the if
that checks inspectedModules and strings.Contains(cleanImp, ".") in run.go:
explain that the branch skips external (dot-qualified) imports that weren't
inspected to avoid "imported and not used" errors, matching the identical logic
in build.go; keep the existing condition (if !inspectedModules[cleanImp] &&
strings.Contains(cleanImp, ".") { continue }) unchanged and only add the
one-line comment for maintainability and clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d8555b8-5c94-407a-a181-108123314ba8
⛔ Files ignored due to path filters (1)
examples/projects/link-shortener/typego.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
examples/projects/link-shortener/typego.modules.jsonpkg/cli/cmd/build.gopkg/cli/cmd/run.go
✅ Files skipped from review due to trivial changes (1)
- examples/projects/link-shortener/typego.modules.json
Optimization to
bridge/coreresolving memory bloat (Optimization Debt) resulting from repetitive allocation overhead during Go to Javascript bridge reflection mapping. Tested natively yielding massive throughput improvements.PR created automatically by Jules for task 10221715549259321240 started by @repyh
Summary by CodeRabbit
New Features
Refactor
Chore