Repository navigation
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change reduces lockfile reads when the pnpm version is looked up and keeps the existing behavior for missing files and parsing. No actionable merge risk remains. Pre-merge checks |
|
Summary
Stop reading
pnpm-lock.yamlonce the chunk containing the leading environment-document separator has been read. This is a follow-up to #72: that change already avoids parsing the dependency document, butreadFileSync(..., 'utf8')still reads and decodes the complete lockfile and normalizes every CRLF pair before extracting the small environment document.For a large dependency lockfile with a small environment document, the lookup now reads 64 KiB instead of the entire file. For example, the generated 53.47 MiB LF fixture went from 7.127 ms and 107.98 MiB added peak RSS to 0.095 ms and 0.94 MiB. The real n8n dependency document, wrapped with a controlled environment prefix, went from 0.494 ms to 0.113 ms.
The main justification is avoiding allocations and reads proportional to dependency-document size. These are lookup benchmarks, not a claim about total setup-step time; downloading and installing pnpm usually dominates that time.
Implementation
openSync/readSync/closeSyncloop, preserving the synchronous caller.StringDecoderto handle UTF-8 characters split across reads.finally, including early returns and read failures.extractEnvDocumentand YAML parsing code. There is no document-size cap.dist/index.js; the bundle grows by 503 bytes. No dependencies are added.The reader may include dependency data from the final 64 KiB chunk. Files without an environment header return early; env-only files must still be read to EOF. Cost therefore scales with environment-document size plus at most one read chunk, rather than the complete dependency document.
Compatibility and pnpm comparison
The existing behavior is retained for:
includePrerelease: true).pnpm's own reader also uses 64 KiB reads,
StringDecoder, and early exit on a literal document separator. This implementation keeps setup's synchronous API and existing extractor rather than importing a new dependency or copying the entire upstream helper.Upstream pnpm additionally requires a root-importer configDependencies marker when identifying environment-only documents. This is a literal text check, not comprehensive schema validation. Setup intentionally preserves its existing, more permissive version-selection behavior. Aligning these rules is outside the scope of this performance change.
Document boundaries continue to follow pnpm's literal separator handling rather than general YAML syntax. Separator comments, indentation, and bare CR line endings are not supported.
Benchmark results
Each pair is before → after. RSS is the increase in process peak resident memory measured around one lookup, not total process memory.
Method
a0881977cd8ea73b1ed212236c99ca10dcc80cf4. After: this change. Both readers bundled with the same installed esbuild, YAML, and semver versions.readLockedPnpmVersion(directory, '12')lookup, including YAML parsing and range selection; exclude module startup.process.resourceUsage().maxRSS; also record post-call heap and ArrayBuffer deltas. Post-call heap alone misses transient allocations.Larger structured environment documents
An additional comparison used generated documents following pnpm's environment shape (
configDependencies,packageManagerDependencies,packages, andsnapshots) ahead of n8n's real dependency document. Five warmups and median of three batches of ten lookups were used for these more expensive inputs.Parsing the larger environment dominates both implementations. The 5,000-package case is an upper stress workload, not a claim about typical environment graphs. Its 2,757,900-byte LF file read 1,114,112 bytes with this implementation: exactly the 17 chunks needed to reach its separator at byte 1,098,027.
Tests and validation
All existing tests are retained. Four focused regression tests were added for:
Validation completed:
pnpm exec tsc --noEmit: passed.pnpm build: passed; action bundle targets Node 24.package.yaml+ symlink + BOM/CRLF + registry mirror. Base and changed implementations produced identical results.The full suite is not green locally:
pnpm testreported 103 passed / 4 failed. The unchanged base, archived into a separate directory, reproduced the same four failures (99 passed / 4 failed). Running the changed suite outside the sandbox also reproduced them. They are thesrc/pnpm-commands.test.mjscases for:The failures involve absent command-fixture output and workspace-root selection. Their precise local cause was not established; they are not attributed to this PR.
Source/test whitespace checks pass. A full
git diff --checkreports two trailing-whitespace lines in dependency template literals in the regenerated bundle; the original bundle contains the same template-literal whitespace.Tradeoffs and remaining coverage
Reproducing the main benchmark
The following creates before/after reader bundles and benchmark inputs in a temporary directory. Run from this PR checkout with dependencies installed and Node supporting
--expose-gc.Commands and standalone benchmark harness
Save the following as
$BENCH_DIR/benchmark.mjs, then runnode "$BENCH_DIR/benchmark.mjs"from the checkout. Results are printed and written to$BENCH_DIR/results.json.Summary by CodeRabbit