perf(bridge): daily optimizations for bridge/core and eventloop#149
perf(bridge): daily optimizations for bridge/core and eventloop#149repyh wants to merge 2 commits into
Conversation
- Optimized `bindMap` using `v.MapRange()` to avoid `MapKeys()` slice allocation. - Replaced slow `fmt.Sprint` with fast `strconv` formatting for integer map keys. - Optimized `bindSlice` with a fast-path for `[]byte`, delegating to a new `ToUint8Array` helper in `arraybuffer.go`. - Fixed a concurrency panic bug in `EventLoop.CreatePromise` by ensuring `wg.Done()` is bounded by `sync.Once`. - Improved struct packing in `EventLoop` by grouping boolean fields and `sync.Mutex` for better 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 pull request adds a new ChangesByte Array and Map Binding Optimization
Event Loop Promise Settlement Synchronization
CLI Import Inspection
Example Manifest Pin
Sequence Diagram(s)sequenceDiagram
participant ResolveHandler
participant RejectHandler
participant Once
participant EventLoop
participant WaitGroup
ResolveHandler->>Once: Do(func){...}
Once-->>ResolveHandler: first-call runs
ResolveHandler->>EventLoop: RunOnLoop(resolve)
RejectHandler->>Once: Do(func){...}
Once-->>RejectHandler: subsequent-call no-op
RejectHandler->>EventLoop: RunOnLoop(reject)
Once->>WaitGroup: el.wg.Done() (once)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 (2)
eventloop/eventloop.go (1)
124-152: 💤 Low value
sync.Onceguard correctly fixes the double-wg.Done()panic.The shared
onceacross bothresolveandrejectis the right granularity: the singleel.wg.Add(1)at line 128 is paired with at most oneel.wg.Done()regardless of how many times either callback fires (or whether both fire). Sobek'sres/rejare themselves no-ops on an already-settled promise, so JS-level semantics stay consistent too.One small robustness nit you may want to consider: if
res(v)(orrej(v)) panics inside the job, control exits the closure before reachingonce.Do, leaving the promise'swgcounter elevated and hangingShutdown/autostop. Adefermakes this resilient without changing the happy path:♻️ Optional: defer the once.Do to survive panics in res/rej
resolve = func(v interface{}) { el.RunOnLoop(func() { + defer once.Do(func() { + el.wg.Done() + }) _ = res(v) - once.Do(func() { - el.wg.Done() - }) }) } reject = func(v interface{}) { el.RunOnLoop(func() { + defer once.Do(func() { + el.wg.Done() + }) _ = rej(v) - once.Do(func() { - el.wg.Done() - }) }) }Note this only mitigates panics inside
res/rej; a panic-resilientel.wg.Done()for the outer job inStart's loop (line 71) is a separate, pre-existing concern and out of scope here.🤖 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 124 - 152, The current CreatePromise uses a shared sync.Once to ensure el.wg.Done() runs at most once, but if res(v) or rej(v) panics the closure returns before once.Do runs leaving the waitgroup count stuck; modify the resolve and reject closures (the functions created inside CreatePromise) so that inside the el.RunOnLoop callback you call defer once.Do() at the top of the closure and then invoke res(v) or rej(v), ensuring the once.Do() runs even if res/rej panics while preserving the single-call semantics of the shared once and the existing el.wg.Add(1) pairing.bridge/core/reflection.go (1)
346-353: ⚡ Quick winSimplify the unaddressable array handling.
In the else branch (line 346),
v.Kind()is guaranteed to bereflect.Arraybecause:
bindSliceis only called for slices or arrays (line 69)- The condition on line 344 excluded slices
- Therefore
vmust be an unaddressable arrayThe check on line 349 is always true, and the else branch (line 352) is unreachable. Additionally, the type assertion on line 350 could theoretically panic.
♻️ Simplified implementation
} else { + // v is an unaddressable array; create an addressable copy vCopy := reflect.New(v.Type()).Elem() reflect.Copy(vCopy, v) - if vCopy.Kind() == reflect.Array { - b = vCopy.Slice(0, vCopy.Len()).Interface().([]byte) - } else { - b = vCopy.Bytes() - } + // Slice the array to get a []byte slice, then extract via Bytes() + sliceVal := vCopy.Slice(0, vCopy.Len()) + b = sliceVal.Bytes() }🤖 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 346 - 353, The branch in bindSlice that handles unaddressable arrays can be simplified: remove the unreachable vCopy.Kind() == reflect.Array check and the unsafe []byte type assertion; after creating vCopy := reflect.New(v.Type()).Elem() and reflect.Copy(vCopy, v), create sliceValue := vCopy.Slice(0, vCopy.Len()) and then if sliceValue.Type().Elem().Kind() == reflect.Uint8 convert safely to []byte (e.g., via a safe type assertion with ok := sliceValue.Interface().([]byte)) and assign to b, otherwise assign b = sliceValue.Interface(); this removes the dead branch and avoids a possible panic while keeping handling for non-byte arrays.
🤖 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 346-353: The branch in bindSlice that handles unaddressable arrays
can be simplified: remove the unreachable vCopy.Kind() == reflect.Array check
and the unsafe []byte type assertion; after creating vCopy :=
reflect.New(v.Type()).Elem() and reflect.Copy(vCopy, v), create sliceValue :=
vCopy.Slice(0, vCopy.Len()) and then if sliceValue.Type().Elem().Kind() ==
reflect.Uint8 convert safely to []byte (e.g., via a safe type assertion with ok
:= sliceValue.Interface().([]byte)) and assign to b, otherwise assign b =
sliceValue.Interface(); this removes the dead branch and avoids a possible panic
while keeping handling for non-byte arrays.
In `@eventloop/eventloop.go`:
- Around line 124-152: The current CreatePromise uses a shared sync.Once to
ensure el.wg.Done() runs at most once, but if res(v) or rej(v) panics the
closure returns before once.Do runs leaving the waitgroup count stuck; modify
the resolve and reject closures (the functions created inside CreatePromise) so
that inside the el.RunOnLoop callback you call defer once.Do() at the top of the
closure and then invoke res(v) or rej(v), ensuring the once.Do() runs even if
res/rej panics while preserving the single-call semantics of the shared once and
the existing el.wg.Add(1) pairing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19813a31-c870-4034-a3b3-cfe259129f61
📒 Files selected for processing (3)
bridge/core/arraybuffer.gobridge/core/reflection.goeventloop/eventloop.go
…-not-used errors Reverted accidental Go version bump in CLI templates that caused CI failures. Added a check in `build.go` and `run.go` to only write `importBlock` entries for successfully inspected modules, eliminating 'imported and not used' compiler errors when inspection fails. Co-authored-by: repyh <63894915+repyh@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/cli/cmd/build.go (1)
103-103: ⚡ Quick winConsider logging when import inspection fails.
Silently omitting imports that fail
linker.Inspectcan lead to confusing downstream Go compiler errors. This is especially inconsistent with lines 68-72 where inspection failures are explicitly logged with a warning.🔍 Proposed improvement
- if _, err := linker.Inspect(cleanImp, fetcher.TempDir); err == nil { importBlock.WriteString(fmt.Sprintf("\t\"%s\"\n", cleanImp)) } + if _, err := linker.Inspect(cleanImp, fetcher.TempDir); err == nil { + importBlock.WriteString(fmt.Sprintf("\t\"%s\"\n", cleanImp)) + } else { + fmt.Printf("Warning: Skipping import %s during import block generation (inspection failed: %v)\n", cleanImp, err) + }🤖 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` at line 103, The import inspection currently drops failures silently in the line calling linker.Inspect(cleanImp, fetcher.TempDir); change it to log the error (using the same warning/log method used earlier around lines 68-72) when err != nil so failed inspections are visible, while preserving the existing behavior of only writing successful imports to importBlock via importBlock.WriteString(fmt.Sprintf("\t\"%s\"\n", cleanImp)). Ensure you reference the same logger/warn function used elsewhere so messages are consistent and include the import name (cleanImp) and the error.pkg/cli/cmd/run.go (1)
128-128: ⚡ Quick winConsider logging when import inspection fails.
Silently omitting imports that fail
linker.Inspectcan lead to confusing downstream Go compiler errors when the generated shim references packages that aren't in the import block. Adding a warning or error message when inspection fails would improve debuggability.🔍 Proposed improvement
- if _, err := linker.Inspect(cleanImp, fetcher.TempDir); err == nil { importBlock.WriteString(fmt.Sprintf("\t\"%s\"\n", cleanImp)) } + if _, err := linker.Inspect(cleanImp, fetcher.TempDir); err == nil { + importBlock.WriteString(fmt.Sprintf("\t\"%s\"\n", cleanImp)) + } else { + fmt.Printf("Warning: Skipping import %s (inspection failed: %v)\n", cleanImp, err) + }🤖 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` at line 128, The current call to linker.Inspect(cleanImp, fetcher.TempDir) silently ignores errors; update the conditional around linker.Inspect in the code that builds importBlock so that when err != nil you emit a warning including the package name (cleanImp) and the error value (err) rather than doing nothing; keep the existing success branch that writes to importBlock and use the project's logging facility (or fmt/log to stderr if no logger exists) so the warning makes it clear which import failed and why.
🤖 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 `@pkg/cli/cmd/build.go`:
- Around line 100-102: The skip list in build.go (the cleanImp check) is
inconsistent with run.go; update build.go to either mirror run.go by skipping
"sync", "net/http", and "memory" in addition to "fmt" and "os" (so the
bridge-handled internal modules are consistently excluded), or add a clear
comment above the cleanImp check explaining why build.go intentionally only
skips "fmt" and "os" while run.go skips the additional bridge modules; reference
the cleanImp variable and the ShimTemplate import behavior when making the
change so the intent is obvious and consistent across build.go and run.go.
---
Nitpick comments:
In `@pkg/cli/cmd/build.go`:
- Line 103: The import inspection currently drops failures silently in the line
calling linker.Inspect(cleanImp, fetcher.TempDir); change it to log the error
(using the same warning/log method used earlier around lines 68-72) when err !=
nil so failed inspections are visible, while preserving the existing behavior of
only writing successful imports to importBlock via
importBlock.WriteString(fmt.Sprintf("\t\"%s\"\n", cleanImp)). Ensure you
reference the same logger/warn function used elsewhere so messages are
consistent and include the import name (cleanImp) and the error.
In `@pkg/cli/cmd/run.go`:
- Line 128: The current call to linker.Inspect(cleanImp, fetcher.TempDir)
silently ignores errors; update the conditional around linker.Inspect in the
code that builds importBlock so that when err != nil you emit a warning
including the package name (cleanImp) and the error value (err) rather than
doing nothing; keep the existing success branch that writes to importBlock and
use the project's logging facility (or fmt/log to stderr if no logger exists) so
the warning makes it clear which import failed and why.
🪄 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: bc4d61d5-f6de-4211-be22-7ddbad5a9861
⛔ Files ignored due to path filters (1)
examples/projects/link-shortener/typego.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
examples/projects/link-shortener/service_binexamples/projects/link-shortener/typego.modules.jsonpkg/cli/cmd/build.gopkg/cli/cmd/run.gotypego
✅ Files skipped from review due to trivial changes (1)
- examples/projects/link-shortener/typego.modules.json
| if cleanImp == "fmt" || cleanImp == "os" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if sync, net/http, or memory are in the shim template or hardcoded elsewhere
# Search for references to these packages in builder templates
rg -n "sync|net/http|memory" --type go -g '*builder*' -g '*shim*' -C 2
# Also check if these are imported in the shimTemplate constant
rg -n "ShimTemplate" --type go -A 30Repository: repyh/typego
Length of output: 7721
🏁 Script executed:
# Get the actual skip list code from build.go around lines 100-102
sed -n '95,110p' pkg/cli/cmd/build.goRepository: repyh/typego
Length of output: 708
🏁 Script executed:
# Get the skip list code from run.go around lines 95-98
sed -n '90,105p' pkg/cli/cmd/run.goRepository: repyh/typego
Length of output: 720
🏁 Script executed:
# Get the complete ShimTemplate from templates.go
cat internal/builder/templates.goRepository: repyh/typego
Length of output: 1729
Confirm skip list inconsistency between build.go and run.go.
The skip lists differ: build.go (lines 100-102) skips only fmt and os, while run.go (lines 95-98) also skips sync, net/http, and memory.
The ShimTemplate only hardcodes fmt and os as imports, which aligns with build.go's minimal skip list. However, run.go's additional skips are marked as "internal modules handled by bridge" but are not reflected in build.go. If these bridge modules should be consistently unavailable across both commands, the skip lists should be unified; if the difference is intentional due to command context, add a clarifying comment to build.go explaining why it excludes these additional packages.
🤖 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 100 - 102, The skip list in build.go (the
cleanImp check) is inconsistent with run.go; update build.go to either mirror
run.go by skipping "sync", "net/http", and "memory" in addition to "fmt" and
"os" (so the bridge-handled internal modules are consistently excluded), or add
a clear comment above the cleanImp check explaining why build.go intentionally
only skips "fmt" and "os" while run.go skips the additional bridge modules;
reference the cleanImp variable and the ShimTemplate import behavior when making
the change so the intent is obvious and consistent across build.go and run.go.
1 participant
This patch addresses the daily performance audit for the
bridge/coreandeventloopmodules.Key improvements:
bindMapby utilizingMapRangeand fast-pathing integer conversions.ToUint8Arraypath for[]byteslice bindings, safely guarding against underlying array mutations while preventing element-by-element generic looping overhead.EventLoopdue to un-guardedwg.Done()calls from promises.EventLoopfor optimal CPU cache line locality.PR created automatically by Jules for task 2092847080783815077 started by @repyh
Summary by CodeRabbit
Bug Fixes
Chores