perf: [AI-written] 5 performance optimizations#58
Conversation
…mpling Removed the 1-second time.Sleep inside getBandwidthSpeed() that was blocking the system stats collection goroutine. Replaced with persistent package-level state (lastRxBytes, lastTxBytes, lastTime) that computes bandwidth deltas from cumulative network counters on each call. This eliminates the CPU wait-state caused by the sleep, reduces the effective stats collection cycle from ~2.5s to 1.5s, and keeps the goroutine fully non-blocking.
…ontention Replaced the long critical section in Config.ToBytes() that held read locks on ALL inbound mutexes during JSON serialization. Now each inbound is snapshotted individually under a microsecond lock/unlock, producing a detached Inbound copy. The heavy json.Marshal runs on the detached copy with zero locks held. This prevents concurrent SyncUser API calls from being blocked by config serialization, reducing lock contention on multi-inbound configurations.
… pressure Added a package-level sync.Pool that recycles the 64KB bufio.Scanner buffer across Xray process restarts instead of allocating a fresh buffer on every captureProcessLogs() call. This eliminates one 64KB heap allocation per Start() invocation, reducing GC pressure on memory-constrained VPS instances.
…timize startup context core.go: Added cleanExit flag to Core struct. When Xray exits cleanly (expected shutdown), the flag is set so the next Start() skips the expensive /proc or ps-based orphan process scan. The scan only runs when Xray crashed unexpectedly, avoiding unnecessary CPU spikes on normal restarts. controller.go: Removed the redundant context.WithCancel() allocation in New() that was immediately discarded and replaced in Connect(). Added a nil-guard on cancelFunc in Disconnect() for safe pre-Connect teardown.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThis PR makes five independent improvements: refactors Xray config serialization to use read-locked snapshots instead of mutating live ChangesBackend and System Improvements
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 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/xray/config.go (1)
327-330: 💤 Low valueShallow copy of
Settingsmap shares nested references.The for-range copy creates a new map but values remain shared references. If any value is a mutable type (nested map/slice) that gets modified concurrently elsewhere, the snapshot could observe partial updates. This appears acceptable given that Settings values are typically primitives or read-only config data, but worth noting if the data model evolves.
🤖 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 `@backend/xray/config.go` around lines 327 - 330, The for-range copy of inbound.Settings creates a new map but leaves values as shared references, which could cause issues if any value is a mutable type (nested map/slice) that gets modified concurrently. While this appears acceptable since Settings values are typically primitives or read-only config data, add a comment above the snap.Settings map copy loop in the range assignment block explaining this shallow copy behavior and the conditions under which it would need to be changed to a deep copy (i.e., if the data model evolves to include mutable nested types).
🤖 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 `@backend/xray/config.go`:
- Around line 425-428: Add a nil check for snapConfig.LogConfig before
attempting to access the AccessLog and ErrorLog fields on lines 425-428. Wrap
the code block that accesses snapConfig.LogConfig.AccessLog and
snapConfig.LogConfig.ErrorLog with a guard condition that verifies
snapConfig.LogConfig is not nil first. This prevents a nil dereference panic
when the parsed config JSON lacks a log section.
- Around line 425-433: The snapConfig.LogConfig pointer references the live
c.LogConfig object, causing mutations during serialization to race with
concurrent readers and creating a nil dereference risk if c.LogConfig is not
initialized. Replace the approach of temporarily blanking the AccessLog and
ErrorLog fields by instead creating a copy of the LogConfig struct within the
snapshot with the log fields set to empty strings. Add a nil guard to check if
c.LogConfig exists before attempting to copy it, returning appropriate handling
(such as using an empty LogConfig struct or skipping the copy) when c.LogConfig
is nil. This eliminates both the race condition and the potential panic from
dereferencing a nil LogConfig pointer.
In `@controller/controller.go`:
- Around line 68-70: The `Disconnect()` method reads `c.cancelFunc` without
holding the lock `c.mu`, while the `Connect()` method writes to `c.cancelFunc`
under lock synchronization. This creates a data race on concurrent access. Wrap
the read of `c.cancelFunc` in the `Disconnect()` method (the nil check and the
call to `c.cancelFunc()`) by acquiring `c.mu` lock before the access and
releasing it after, matching the synchronization pattern used in `Connect()` to
ensure thread-safe access to this shared field.
---
Nitpick comments:
In `@backend/xray/config.go`:
- Around line 327-330: The for-range copy of inbound.Settings creates a new map
but leaves values as shared references, which could cause issues if any value is
a mutable type (nested map/slice) that gets modified concurrently. While this
appears acceptable since Settings values are typically primitives or read-only
config data, add a comment above the snap.Settings map copy loop in the range
assignment block explaining this shallow copy behavior and the conditions under
which it would need to be changed to a deep copy (i.e., if the data model
evolves to include mutable nested types).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c115af5-bfc3-4639-b127-91462ae04ae8
📒 Files selected for processing (5)
backend/xray/config.gobackend/xray/core.gobackend/xray/log.gocontroller/controller.gopkg/sysstats/sys.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 `@controller/controller.go`:
- Around line 68-84: The Disconnect() method has duplicate code blocks that
cause compilation errors. Remove the second backend variable declaration block
starting at line 81 (the duplicate c.mu.Lock(), backend := c.backend,
c.mu.Unlock() sequence) to eliminate the duplicate `:=` assignment of the
backend variable in the same scope and complete the Disconnect() function
properly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da52e867-2f31-426c-a11c-5a9bad2d1be9
📒 Files selected for processing (1)
controller/controller.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@controller/controller.go`:
- Around line 68-84: The Disconnect() method has duplicate code blocks that
cause compilation errors. Remove the second backend variable declaration block
starting at line 81 (the duplicate c.mu.Lock(), backend := c.backend,
c.mu.Unlock() sequence) to eliminate the duplicate `:=` assignment of the
backend variable in the same scope and complete the Disconnect() function
properly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da52e867-2f31-426c-a11c-5a9bad2d1be9
📒 Files selected for processing (1)
controller/controller.go
🛑 Comments failed to post (1)
controller/controller.go (1)
68-84:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove duplicated
Disconnect()block — current code does not compile.At Line 68,
Disconnect()is declared a second time inside itself, which causes the parser error reported by typecheck. Also, Line 83 re-declaresbackendwith:=in the same scope as Line 79, which is another compile-time failure. Keep only one function header and one backend snapshot block.Proposed minimal fix
func (c *Controller) Disconnect() { -func (c *Controller) Disconnect() { c.mu.Lock() cancel := c.cancelFunc c.cancelFunc = nil c.mu.Unlock() if cancel != nil { cancel() } c.mu.Lock() backend := c.backend c.mu.Unlock() - - c.mu.Lock() - backend := c.backend - c.mu.Unlock() // Shutdown backend outside of lock to avoid deadlock // Shutdown() will wait for process termination to complete if backend != nil { backend.Shutdown()🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 68-68: : # github.com/pasarguard/node/controller [github.com/pasarguard/node/controller.test]
controller/controller.go:68:35: syntax error: unexpected { at end of statement(typecheck)
[error] 68-68: expected ';', found '{'
(typecheck)
🤖 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 `@controller/controller.go` around lines 68 - 84, The Disconnect() method has duplicate code blocks that cause compilation errors. Remove the second backend variable declaration block starting at line 81 (the duplicate c.mu.Lock(), backend := c.backend, c.mu.Unlock() sequence) to eliminate the duplicate `:=` assignment of the backend variable in the same scope and complete the Disconnect() function properly.Source: Linters/SAST tools
SnapshotToBytes: replaced shared pointer copy of LogConfig with a value copy. When c.LogConfig is non-nil, the struct is dereferenced and copied locally so the AccessLog/ErrorLog fields can be blanked without mutating the live object. When c.LogConfig is nil, the field is left nil and skipped, preventing a nil pointer dereference panic. Also added a shallow-copy comment on the Settings map loop documenting the current safety assumption and the conditions that would require a deep copy in the future.
wahh3b-lgtm
changed the title
…d fix cancelFunc race CodeRabbit's auto-commit duplicated the Disconnect() function signature and the backend read block. Removed the duplicates. The cancelFunc race fix is retained: read under lock, nil out, call outside lock.
1 participant
Summary
This PR implements 5 targeted performance optimizations for PasarGuard Node, designed for deployment on small 1-Core / 2GB RAM VPS instances where CPU spikes, GC pressure, and lock contention degrade performance under high traffic.
Changes
1. Non-blocking bandwidth sampling (
pkg/sysstats/sys.go)time.SleepinsidegetBandwidthSpeed()that blocked the stats collection goroutine2. Lock-free config serialization (
backend/xray/config.go)ToBytes()no longer holds read locks on ALL inbound mutexes during JSON marshalingjson.Marshalruns on a detached copy with zero locks heldSyncUserAPI calls from being blocked by serialization3. sync.Pool for scanner buffers (
backend/xray/log.go)sync.Poolthat recycles the 64KBbufio.Scannerbuffer across Xray restartsStart()invocation4. Conditional orphan process scan (
backend/xray/core.go)cleanExitflag toCorestruct/procorps-based global process scan when Xray exited cleanly5. Eliminated unused context allocation (
controller/controller.go)context.WithCancel()inNew()that was immediately discardedcancelFuncinDisconnect()for safe pre-Connect teardownTest Results
go build ./...compiles successfully (exit code 0)xray/api,wireguard,controller,pkg/statsImpact
Summary by CodeRabbit