perf: eliminate redundant syscalls in JSON serialization and filesystem operations - #223
Conversation
🧪 Testing GuideWhat this PR addressesThis PR optimizes the filesystem API hot path in two ways:
Steps to reproduce / exercise the changesSince this is a performance optimization with no intended behavior change, focus on verifying correctness:
What to verify (expected behavior)
Note Posted by PR Testing Guide · Tag @mendral-app with feedback. |
🔍 Interaction Flow DiagramHere's a sequence diagram showing how the optimized components interact after this PR: sequenceDiagram
participant Client
participant Gin as Gin Router
participant Handler as handler (base.go)
participant FS as filesystem handler
participant Jsoniter as jsoniter
participant OS as OS / Syscalls
Note over Handler: Response Path (optimized)
Client->>Gin: HTTP Request
Gin->>Handler: Route dispatch
Handler->>Jsoniter: BindJSON (NewDecoder)
Jsoniter-->>Handler: Decoded request body
alt ListDirectory / ReadFile / GetFileInfo
Handler->>FS: Handle filesystem operation
FS->>OS: os.ReadDir() / os.Lstat() (single call)
OS-->>FS: DirEntry[] / FileInfo
FS->>FS: entry.IsDir() (cached, no syscall)
FS->>FS: getOwnerAndGroupFromInfo(info)
Note right of FS: Reuses existing FileInfo<br/>instead of redundant os.Lstat()
FS-->>Handler: Response payload
end
Handler->>Handler: writeJSONResponse()
Handler->>Jsoniter: json.Marshal(payload)
Jsoniter-->>Handler: []byte
Handler->>Gin: Write status + JSON body directly
Gin-->>Client: HTTP Response
Summary of the Flow
The key insight is eliminating redundant syscalls by reusing Note Posted by PR Sequence Diagram · Tag @mendral-app with feedback. |
|
📋 Created Linear issue ENG-3016 — status: In Progress
Auto-created because no Linear reference was found in the PR title, description, or branch name. Note Posted by Linear Issue Enforcer · Tag @mendral-app with feedback. |
![mendral-app[bot]](https://avatars.githubusercontent.com/in/1975249?s=60&v=4){kind=small}
There was a problem hiding this comment.
Needs attention — 1 issue in 1 file
The filesystem syscall elimination is correct and well-reasoned. The BindJSON change silently drops Gin's struct validation (binding:"required" tags) — multiple request structs like DriveMountRequest, ProcessRequest, and ApplyEditRequest have binding:"required" fields that will no longer be enforced. Even if most callers already used the custom BindJSON, at least drive.go and system.go use c.ShouldBindJSON() directly, confirming that validation was intentionally relied upon somewhere in the codebase.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<assessment>
The filesystem syscall elimination is correct and well-reasoned. The `BindJSON` change silently drops Gin's struct validation (`binding:"required"` tags) — multiple request structs like `DriveMountRequest`, `ProcessRequest`, and `ApplyEditRequest` have `binding:"required"` fields that will no longer be enforced. Even if most callers already used the custom `BindJSON`, at least `drive.go` and `system.go` use `c.ShouldBindJSON()` directly, confirming that validation was intentionally relied upon somewhere in the codebase.
</assessment>
<file name="sandbox-api/src/handler/base.go">
<issue location="sandbox-api/src/handler/base.go:97">
Switching from `ShouldBindJSON` to raw `json.NewDecoder().Decode()` silently drops Gin's struct validation. Any request struct with `binding:"required"` tags (e.g., `ProcessRequest.Command`, `DriveMountRequest.DriveName`) will no longer have those constraints enforced, allowing invalid requests through.
</issue>
</file>
Tag @mendral-app with feedback or questions. View session
| decoder := json.NewDecoder(c.Request.Body) | ||
| if err := decoder.Decode(obj); err != nil { | ||
| return fmt.Errorf("invalid request body: %w", err) |
There was a problem hiding this comment.
bug (P2): Switching from ShouldBindJSON to raw json.NewDecoder().Decode() silently drops Gin's struct validation. Any request struct with binding:"required" tags (e.g., ProcessRequest.Command, DriveMountRequest.DriveName) will no longer have those constraints enforced, allowing invalid requests through.
Suggested change
| decoder := json.NewDecoder(c.Request.Body) | |
| if err := decoder.Decode(obj); err != nil { | |
| return fmt.Errorf("invalid request body: %w", err) | |
| func (h *BaseHandler) BindJSON(c *gin.Context, obj interface{}) error { | |
| if err := json.NewDecoder(c.Request.Body).Decode(obj); err != nil { | |
| return fmt.Errorf("invalid request body: %w", err) | |
| } | |
| if err := binding.Validator.ValidateStruct(obj); err != nil { | |
| return fmt.Errorf("invalid request body: %w", err) | |
| } | |
| return nil | |
| } |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sandbox-api/src/handler/base.go, line 97:
<issue>
Switching from `ShouldBindJSON` to raw `json.NewDecoder().Decode()` silently drops Gin's struct validation. Any request struct with `binding:"required"` tags (e.g., `ProcessRequest.Command`, `DriveMountRequest.DriveName`) will no longer have those constraints enforced, allowing invalid requests through.
</issue>
There was a problem hiding this comment.
Let's break these JSON-related changes into a separate PR. They seem completely unrelated to the Lstat-related changes.
There was a problem hiding this comment.
That's a fair point. The JSON serialization changes (switching to jsoniter, adding writeJSONResponse(), updating BindJSON() in base.go) and the Lstat optimizations (eliminating redundant syscalls in filesystem.go) are independent optimizations targeting different layers — HTTP serialization vs. filesystem I/O. They don't share any functional dependency.
Splitting them would make each PR easier to review, benchmark independently, and revert if needed.
| // For files and symlinks, we need stat info for metadata | ||
| absEntryPath := filepath.Join(absPath, entry.Name()) | ||
|
|
||
| // Use os.Lstat to get info about the symlink itself, not its target |
There was a problem hiding this comment.
Actually, nevermind, I didn't realize that info is used later on as well.
| decoder := json.NewDecoder(c.Request.Body) | ||
| if err := decoder.Decode(obj); err != nil { | ||
| return fmt.Errorf("invalid request body: %w", err) |
There was a problem hiding this comment.
Let's break these JSON-related changes into a separate PR. They seem completely unrelated to the Lstat-related changes.
1 participant
Fixes ENG-3016
Summary
Two complementary optimizations that reduce redundant work in the hot path of the filesystem API handlers, yielding a measurable improvement on
BenchmarkListDirectory.Changes
1. Replace
encoding/jsonwithjsoniterfor HTTP serialization (base.go)Gin's
c.JSON()uses the standard libraryencoding/jsoninternally. By switching tojsoniter.ConfigCompatibleWithStandardLibraryvia a newwriteJSONResponse()helper, all response serialization bypasses the slower standard library path.2. Eliminate redundant
os.Lstatsyscalls in filesystem operations (filesystem.go)ListDirectory: Eliminates redundantos.Lstat()calls by reusingDirEntry.IsDir()andgetOwnerAndGroupFromInfo(info).ReadFile/GetFileInfo: Pass existingFileInfodirectly, eliminating a redundant syscall per call.Benchmark Results
BenchmarkListDirectoryNote
Performance optimization PR that replaces Gin's
c.JSON()with jsoniter-basedwriteJSONResponse()and eliminates redundantos.Lstatsyscalls in filesystem operations by reusing already-obtainedFileInfo.Written by Mendral for commit 36b473f.