perf: optimize reflection boxing and eventloop sync#151
Conversation
- Refactor `eventloop.EventLoop` struct for better cache locality. - Wrap `wg.Done()` calls in `CreatePromise` with `sync.Once` to prevent panics from multiple resolutions. - Implement `ToUint8Array` in `arraybuffer.go` for safer `Uint8Array` bridging. - Add `[]byte` fast path in `bindSlice` to prevent per-element reflection overhead. - Optimize `bindMap` by replacing `v.MapKeys()` with `v.MapRange()` and using `strconv` for primitive formatting to avoid `fmt.Sprint` interface boxing. 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. |
📝 WalkthroughWalkthroughExposes Go []byte as JS Uint8Array when possible, streams map iteration with targeted key formatting, ensures event-loop promise callbacks decrement the wait-group only once, and updates embedded/generated go.mod Go version pins to 1.24.0. ChangesCore binding and event-loop improvements
Example dependency update
Toolchain go.mod updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bridge/core/reflection.go (1)
341-356: ⚡ Quick winRemove redundant byte slice copy in the []byte fast path.
Sobek's
vm.NewArrayBuffercopies the provided byte slice into ArrayBuffer-owned memory rather than sharing the underlying storage. The intermediate copy at lines 352-354 is therefore unnecessary. Passdatadirectly toToUint8Array:♻️ Simplification
- // Copy the data to prevent JS from mutating the underlying Go memory - cpy := make([]byte, len(data)) - copy(cpy, data) - return ToUint8Array(vm, cpy), nil + // NewArrayBuffer copies the data, preventing JS mutation of Go memory + return ToUint8Array(vm, data), nilThis removes an unnecessary allocation and copy while maintaining the same safety guarantee.
🤖 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 341 - 356, The code currently makes an unnecessary second copy (creating cpy and copy(cpy, data)) before calling ToUint8Array; remove the intermediate copy and pass data directly to ToUint8Array(vm, data). Keep the initial branch that builds data (using v.Bytes() or making a slice and reflect.Copy for unaddressable arrays) but delete the cpy allocation and copy call, and update the comment to reflect that vm.NewArrayBuffer/ToUint8Array already copies the slice into ArrayBuffer-owned memory.
🤖 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 341-356: The code currently makes an unnecessary second copy
(creating cpy and copy(cpy, data)) before calling ToUint8Array; remove the
intermediate copy and pass data directly to ToUint8Array(vm, data). Keep the
initial branch that builds data (using v.Bytes() or making a slice and
reflect.Copy for unaddressable arrays) but delete the cpy allocation and copy
call, and update the comment to reflect that vm.NewArrayBuffer/ToUint8Array
already copies the slice into ArrayBuffer-owned memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be82fce4-4b8c-46bb-848f-51610a5dd0d4
📒 Files selected for processing (3)
bridge/core/arraybuffer.gobridge/core/reflection.goeventloop/eventloop.go
- Pinned `github.com/gin-gonic/gin` to `v1.11.0` in the `link-shortener` example's `typego.modules.json`. The latest version (`v1.12.0`) requires Go 1.25, which is incompatible with the project's Go 1.24 runtime requirement. Co-authored-by: repyh <63894915+repyh@users.noreply.github.com>
- Bumps the generated `go.mod` files in `pkg/cli/cmd/build.go` and `pkg/cli/cmd/run.go` to use `go 1.24.0` instead of `1.23.6`. This allows `typego` to compile projects using `github.com/gin-gonic/gin@latest` (v1.12.0) which recently raised its minimum required Go version to 1.25 (and works gracefully with 1.24 with toolchain upgrades when permitted by the base `go.mod` declaration). - Previously, a hardcoded `go 1.23.6` in the scaffold prevented TypeGo projects from cleanly installing `gin` during the build/run phases in CI. Co-authored-by: repyh <63894915+repyh@users.noreply.github.com>
- Edits `go.mod` files using `go mod edit -go=1.24.0` during dummy module creation (in `internal/linker/fetcher.go`) and internal module initialization (in `internal/ecosystem/resolver/toolchain.go`). - This fixes issues where module inspection (like for `gin@v1.12.0`) fails because the default `go.mod` version falls back to a lower version that does not satisfy the module's minimum Go 1.25 requirement, but the underlying toolchain supports Go 1.24+ module version pinning logic. Co-authored-by: repyh <63894915+repyh@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/ecosystem/resolver/toolchain.go`:
- Around line 56-60: The InitGoMod function currently ignores the result of
editCmd.Run() (the editCmd that runs "go mod edit -go=1.24.0"), so failures to
pin the go version are discarded; change this to capture the error from
editCmd.Run(), check it, and return or surface it (e.g., wrap and return the
error from InitGoMod) instead of assigning to `_`; ensure you reference the
existing editCmd variable and propagate the error out of InitGoMod so callers
know the command failed.
In `@internal/linker/fetcher.go`:
- Around line 28-31: In NewFetcher, don't drop the error from the
exec.Command("go", "mod", "edit", "-go=1.24.0") call (editCmd); instead run
editCmd.CombinedOutput() and follow the same error-handling pattern used for the
go mod init block: if it returns an error, return a wrapped fmt.Errorf with a
clear message (e.g., "failed to set go version to 1.24.0") including the
original error and the combined output so failures are surfaced instead of
silently ignored.
🪄 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: 1ce19a19-27fa-4ec2-a175-56ef5ed1a0b3
📒 Files selected for processing (2)
internal/ecosystem/resolver/toolchain.gointernal/linker/fetcher.go
| // Edit go.mod to ensure go version is 1.24.0 (TypeGo min version, also fixes go.mod mismatch with newer modules like gin v1.12.0) | ||
| editCmd := exec.Command("go", "mod", "edit", "-go=1.24.0") | ||
| editCmd.Dir = dir | ||
| _ = editCmd.Run() | ||
|
|
There was a problem hiding this comment.
Handle go mod edit failures instead of discarding them.
At Line 59, _ = editCmd.Run() suppresses failures, so InitGoMod can return nil even when the go 1.24.0 pin was not applied.
Suggested fix
// Edit go.mod to ensure go version is 1.24.0 (TypeGo min version, also fixes go.mod mismatch with newer modules like gin v1.12.0)
editCmd := exec.Command("go", "mod", "edit", "-go=1.24.0")
editCmd.Dir = dir
- _ = editCmd.Run()
+ if out, err := editCmd.CombinedOutput(); err != nil {
+ return fmt.Errorf("go mod edit -go=1.24.0 failed: %w: %s", err, out)
+ }#!/bin/bash
# Read-only verification: find go mod edit pinning calls and ignored Run() results.
rg -nP --type go 'go",\s*"mod",\s*"edit",\s*"-go=1\.24\.0"' -C2
rg -nP --type go '_\s*=\s*[A-Za-z_][A-Za-z0-9_]*\.Run\(\)' -C2🤖 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 `@internal/ecosystem/resolver/toolchain.go` around lines 56 - 60, The InitGoMod
function currently ignores the result of editCmd.Run() (the editCmd that runs
"go mod edit -go=1.24.0"), so failures to pin the go version are discarded;
change this to capture the error from editCmd.Run(), check it, and return or
surface it (e.g., wrap and return the error from InitGoMod) instead of assigning
to `_`; ensure you reference the existing editCmd variable and propagate the
error out of InitGoMod so callers know the command failed.
| // Edit go.mod to ensure go version is 1.24.0 (TypeGo min version, also fixes go.mod mismatch with newer modules) | ||
| editCmd := exec.Command("go", "mod", "edit", "-go=1.24.0") | ||
| editCmd.Dir = tempDir | ||
| _ = editCmd.Run() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the fetcher.go file
find . -name "fetcher.go" -type fRepository: repyh/typego
Length of output: 1927
🏁 Script executed:
# Read the fetcher.go file to see the context around lines 28-31
if [ -f "internal/linker/fetcher.go" ]; then
cat -n "internal/linker/fetcher.go" | head -50
fiRepository: repyh/typego
Length of output: 1616
🏁 Script executed:
# Check if there are other error handling patterns in the same file
if [ -f "internal/linker/fetcher.go" ]; then
wc -l "internal/linker/fetcher.go"
fiRepository: repyh/typego
Length of output: 86
Don't silently ignore go mod edit errors in NewFetcher.
At line 31, the error from go mod edit -go=1.24.0 is dropped with _ = editCmd.Run(), while the go mod init command at lines 24–26 properly checks for errors. This inconsistency creates a silent failure: if setting the go version fails, subsequent operations in the Get method may fail for unclear reasons, making debugging harder.
Use the established error-handling pattern from the same function:
if out, err := editCmd.CombinedOutput(); err != nil {
return nil, fmt.Errorf("failed to set go version to 1.24.0: %w: %s", err, out)
}🤖 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 `@internal/linker/fetcher.go` around lines 28 - 31, In NewFetcher, don't drop
the error from the exec.Command("go", "mod", "edit", "-go=1.24.0") call
(editCmd); instead run editCmd.CombinedOutput() and follow the same
error-handling pattern used for the go mod init block: if it returns an error,
return a wrapped fmt.Errorf with a clear message (e.g., "failed to set go
version to 1.24.0") including the original error and the combined output so
failures are surfaced instead of silently ignored.
1 participant
Performed daily incremental optimization on
bridge/coreandeventloopmodules.Addressed interface boxing bottlenecks, improved cache locality in the event loop, and fixed a concurrency bug in
CreatePromise.PR created automatically by Jules for task 10599774848290073816 started by @repyh
Summary by CodeRabbit
New Features
Improvements
Chores