perf: cache package.json files#7
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a concurrency-safe package.json caching layer (Cache interface and MemoryCache) with GetOrLoad to deduplicate loads, integrates cache into the local Resolver via WithPackageCache and parsePackageJSON, and adds tests verifying functionality, concurrency, and resolver cache usage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MemoryCache
participant Loader
participant FileSystem
Client->>MemoryCache: GetOrLoad(path, loader)
MemoryCache->>MemoryCache: Check Get(path)
alt Cached
MemoryCache-->>Client: Return cached *PackageJSON
else Not cached
alt In-flight for path
MemoryCache-->>Client: Wait for in-flight result
else First loader for path
MemoryCache->>Loader: invoke loader()
Loader->>FileSystem: read package.json (if loader reads disk)
FileSystem-->>Loader: file content
Loader-->>MemoryCache: (*PackageJSON, error)
MemoryCache->>MemoryCache: store result, notify waiters
MemoryCache-->>Client: Return result or error
end
end
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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)packagejson/cache.go (1)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packagejson/cache.go`:
- Around line 83-109: GetOrLoad leaks entries in c.loading because cacheEntry
values are never removed; after entry.once finishes (inside GetOrLoad) delete
the loading map entry for path (use c.loading.Delete(path)) so completed or
failed loads don't accumulate, and update Invalidate to also remove the path
from c.loading in addition to removing from c.cache so subsequent loads create
fresh cacheEntry objects; ensure deletion happens after entry.once.Do returns to
avoid racing the loader.
🧹 Nitpick comments (1)
packagejson/cache_test.go (1)
116-148: Data race:loadCountis accessed from a closure without synchronization.In this single-threaded test, the race is benign because
GetOrLoadis called sequentially. However, if the test were refactored or run with-race, the non-atomic increment inside the closure could be flagged. Consider usingatomic.Int32for consistency with the concurrent test.♻️ Suggested fix using atomic counter
func TestMemoryCacheGetOrLoad(t *testing.T) { cache := packagejson.NewMemoryCache() - loadCount := 0 + var loadCount atomic.Int32 loader := func() (*packagejson.PackageJSON, error) { - loadCount++ + loadCount.Add(1) return &packagejson.PackageJSON{Name: "loaded"}, nil } // First call should invoke loader pkg, err := cache.GetOrLoad("/path/to/package.json", loader) if err != nil { t.Fatalf("GetOrLoad failed: %v", err) } if pkg.Name != "loaded" { t.Errorf("Expected name 'loaded', got %q", pkg.Name) } - if loadCount != 1 { - t.Errorf("Expected loader to be called once, called %d times", loadCount) + if loadCount.Load() != 1 { + t.Errorf("Expected loader to be called once, called %d times", loadCount.Load()) } // Second call should use cached value, not invoke loader pkg, err = cache.GetOrLoad("/path/to/package.json", loader) if err != nil { t.Fatalf("GetOrLoad failed: %v", err) } if pkg.Name != "loaded" { t.Errorf("Expected name 'loaded', got %q", pkg.Name) } - if loadCount != 1 { - t.Errorf("Expected loader to still be called once, called %d times", loadCount) + if loadCount.Load() != 1 { + t.Errorf("Expected loader to still be called once, called %d times", loadCount.Load()) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packagejson/cache.gopackagejson/cache_test.goresolve/local/local.go
🧰 Additional context used
🧬 Code graph analysis (2)
resolve/local/local.go (6)
fs/fs.go (1)
FileSystem(29-46)resolve/resolve.go (2)
Logger(36-39)WorkspacePackage(42-45)resolve/template.go (1)
Template(33-36)importmap/importmap.go (1)
ImportMap(27-37)packagejson/cache.go (1)
Cache(24-38)packagejson/packagejson.go (2)
PackageJSON(32-42)ParseFile(66-72)
packagejson/cache.go (1)
packagejson/packagejson.go (1)
PackageJSON(32-42)
🔇 Additional comments (27)
packagejson/cache_test.go (7)
27-38: LGTM!Clean test for cache miss behavior, correctly verifying both the boolean return and nil package.
40-61: LGTM!Good coverage of the Set/Get roundtrip with field verification.
63-81: LGTM!Proper test flow: set → verify hit → invalidate → verify miss.
83-88: LGTM!Good defensive test ensuring Invalidate doesn't panic on nonexistent keys.
90-109: LGTM!Reasonable concurrency stress test. The done channel pattern works correctly for synchronization.
111-114: LGTM!Compile-time interface conformance check is idiomatic Go.
150-177: LGTM!Excellent concurrent test verifying the single-loader guarantee. Correct use of
atomic.Int32andsync.WaitGroup.packagejson/cache.go (4)
24-38: LGTM!Well-designed interface with clear documentation. The
GetOrLoadmethod is a good addition for atomic cache-or-compute semantics.
40-52: LGTM!Good design separating the main cache map (with RWMutex) from in-flight load coordination (with sync.Map). The
cacheEntrywithsync.Onceensures single-execution semantics.
54-67: LGTM!Constructor and Get method are correctly implemented with proper locking.
69-81: LGTM!Set and Invalidate methods correctly use exclusive locks.
resolve/local/local.go (16)
33-42: LGTM!Clean addition of the
cachefield to the Resolver struct.
57-67: LGTM!Cache field correctly propagated in
WithPackages.
76-85: LGTM!Cache field correctly propagated in
WithTemplate.
91-100: LGTM!Cache field correctly propagated in
WithInputMap.
107-116: LGTM!Cache field correctly propagated in
WithWorkspacePackages.
123-132: LGTM!Cache field correctly propagated in
WithIncludeRootExports.
135-150: LGTM!Well-documented
WithPackageCachemethod following the established builder pattern. Correctly sets the cache field while preserving all other resolver configuration.
152-161: LGTM!Clean helper that transparently uses
GetOrLoadwhen cache is available, falling back to direct parsing otherwise. This centralizes the caching logic effectively.
186-194: LGTM!Root package.json parsing now uses the cached helper.
237-243: LGTM!Direct dependencies now use the cached parsing method.
293-302: LGTM!Workspace package dependencies now use cached parsing.
332-336: LGTM!Workspace node_modules dependencies now use cached parsing.
370-374: LGTM!Workspace package export resolution now uses cached parsing.
457-502: LGTM!Good refactoring of
addPackageToImportMapto build entries locally before merging under the lock, minimizing lock contention. Cache usage is correctly integrated.
547-550: LGTM!Transitive dependency processing now uses cached parsing.
571-575: LGTM!Nested dependency parsing in
processPackageDependenciesParallelnow uses the cache.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Fixes #2
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.