Repository navigation
perf(hasher): reuse resolved dependencies across package targets - #1255
djordjem88 wants to merge 4 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b26a45b-25aa-45e3-95b4-f9a1b55dcb58
Preserve dependency-cache tests alongside supplied metadata tests and their mock cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b26a45b-25aa-45e3-95b4-f9a1b55dcb58
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b26a45b-25aa-45e3-95b4-f9a1b55dcb58
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b26a45b-25aa-45e3-95b4-f9a1b55dcb58
| const internalDeps = Object.keys(allDependencies).filter((dep) => this.packageInfos[dep]); | ||
| const externalDeps = resolveExternalDependencies(allDependencies, this.packageInfos, parsedLock); | ||
| const resolvedDependencies = [...internalDeps, ...externalDeps].sort(); | ||
| const resolvedDependencies = this.getResolvedDependencies(target.packageName!); |
There was a problem hiding this comment.
Maybe its just me but I don't like when the classes private property has the same name as the variable in the function, can we call this variable something different or add __ to it or to the classes private property
There was a problem hiding this comment.
Maybe it's because in some languages a local unqualified reference to a name falls back to the class member? In this case I think renaming the local one to targetResolvedDependencies or just targetDependencies would clarify the code.
There was a problem hiding this comment.
🟡 Changes recommended
The hash test only checks cold-versus-warm consistency, leaving cache-key compatibility regressions undetected without an independently calculated expected hash.
1 open issue
What changed in this PR
Reduces repeated dependency resolution in Lage’s target hashing by reusing each package’s sorted dependency list within a single TargetHasher instance.
Changes:
- Cache internal and external dependency lists, including empty results, without changing input or prerequisite hash calculations.
- Add coverage for reuse, hash consistency, package boundaries, instance isolation, and root targets.
- Include a patch release note for
@lage-run/hasher.
| File | Description |
|---|---|
packages/hasher/src/TargetHasher.ts |
Adds instance-local dependency resolution caching by package. |
packages/hasher/src/__tests__/TargetHasher.test.ts |
Tests dependency reuse, hash behavior, and cache isolation. |
change/change-bddd4ddf-ffd0-4d2e-a084-985058bd7dad.json |
Records the optimization as a patch release. |
Review effort: Max
| await hasher.initialize(); | ||
| const hashes = await Promise.all(targets.map((target) => hasher.hash(target))); | ||
|
|
||
| expect(hashes).toEqual(coldHashes); |
| private globalInputsHash: Record<string, string> | undefined; | ||
| private lockInfo: ParsedLock | undefined; | ||
| private targetHashes: Record<string, string> = {}; | ||
| // Package metadata and the parsed lockfile are fixed for this hasher's lifetime. |
There was a problem hiding this comment.
nit, please make this a doc comment
| // Package metadata and the parsed lockfile are fixed for this hasher's lifetime. | |
| /** Package metadata and the parsed lockfile are fixed for this hasher's lifetime. */ |
Summary
Requested reviewers: Nemanja Tesic (@nemanjatesic) and Elizabeth Craig (@ecraig12345).
Reuse each package's sorted internal and external dependency list across targets within one
TargetHasherinstance. Empty results are cached too. The map is instance-local; target input hashes and cache keys retain their existing calculation.Part of the broader Lage performance effort explored in #1238. This change is self-contained and based on current
main; it does not depend on the other performance PRs.Package metadata and the parsed lockfile retain their existing lifetime. This does not introduce watch-mode manifest/lockfile refresh or a process-global cache.
Validation
mainatcf155216without conflicts; the hasher implementation and tests are unchanged by the sync.