feat: parallel vendor JS download with incremental caching - #77
Conversation
Rewrite vendor.py to use ThreadPoolExecutor for concurrent downloads. Top-level ESM packages and PDF.js files are fetched in parallel, with transitive dependencies resolved level-by-level (BFS). - 3-4x speedup on first-run download (~4s vs ~15s) - SHA-256 content hashing in manifest (v2) for incremental updates - Unchanged files are skipped on re-download - Partial failures don't abort; browser CDN fallback covers gaps - Progress callback for CLI feedback (done/total counter) - Add unit tests for vendor download logic Closes #76
There was a problem hiding this comment.
LGTM ✅ — 架构改得干净,BFS + ThreadPoolExecutor 的拆法比老的递归 _download_esm 好很多。
逐项:
- BFS 层级并行:
_discover_deps和_rewrite_imports分离后,fetch 只做 I/O,dep discovery 和 rewrite 都在主线程跑,url_map/pkg_map不需要锁。正确。 - 增量缓存:SHA-256 hash 比较 → 内容没变的文件跳过写入。v1 manifest 没有 sha256 字段,
old_files.get(name, {}).get("sha256")返回 None,graceful fallback 到全量写入。正确。 - Partial failure:单个包下载失败不 abort,
all_failures收集后 stderr 输出,浏览器 CDN fallback 兜底。correct for a dev tool。 - PDF.js 并行:和 ESM 包在同一个 executor 里并发,不需要等 ESM 先完成。正确。
- Tests:26 个 test 覆盖了 helpers、dep discovery、import rewriting、full download、incremental skip、partial failure、manifest 结构,mock 粒度合适。
没有 blocking issue。
两个 non-blocking:
- Progress 跳跃 — PDF.js futures 在 BFS 循环结束后才 collect,所以 progress counter 会在 ESM 全部完成后突然跳。BFS 发现 transitive deps 时
total_est也会涨,用户看到分母变大。不影响正确性,但 UX 上数字不完全线性。 vendor.py直接print(…, file=sys.stderr)— library 层直接输出到 stderr,比 raise/return failures 让 caller 决定怎么报要 opinionated 一些。当前只有cli.py一个调用者,没有实际问题,但如果以后被当 library 用会 surprise。
There was a problem hiding this comment.
Clean refactor — the recursive-to-BFS transformation is well-structured, the incremental caching logic is correct, and the test coverage is solid. A few observations below, nothing blocking.
Architecture — Separating _discover_deps / _rewrite_imports from the download loop is a nice decomposition. The old recursive _download_esm mixed fetching, parsing, rewriting, and writing into one function; the new design makes each phase independently testable and keeps concurrent fetching on the I/O boundary where it belongs.
Thread safety — Confirmed that url_map/pkg_map mutation only happens in the main thread (inside the as_completed loop after futures resolve), not inside worker callables. _opener is set once before the executor block and only read by workers. All good.
Incremental hash correctness — The SHA-256 is computed on the rewritten ESM content (what's actually on disk), not the raw fetch content, so the cache correctly invalidates when url_map changes between runs (e.g. a new transitive dep alters import paths). PDF.js hashes are on raw binary. Both correct.
Minor nits inline.
| except Exception as e: | ||
| all_failures.append((url, name, e)) | ||
| _report(name) | ||
|
|
There was a problem hiding this comment.
Nit: PDF.js data gets downloaded, hashed, then (if unchanged) discarded — and the manifest loop below re-reads the file from disk and re-hashes it. You could stash the hash from here into a small dict (e.g. pdfjs_hashes[local_name] = file_hash) and look it up during manifest assembly to skip the redundant I/O + hash.
Not a correctness issue, just wasted work on incremental runs.
There was a problem hiding this comment.
Fixed in 6f6be72 — PDF.js hash is now stashed in pdfjs_manifest during the download phase and reused directly in manifest building. No re-read from disk.
| except Exception as e: | ||
| all_failures.append((url, local_name, e)) | ||
| _report(local_name) | ||
|
|
There was a problem hiding this comment.
The print(..., file=sys.stderr) here bypasses the progress callback pattern established for the rest of the function. Consider either:
- Adding an optional
on_warningcallback, or - Using
warnings.warn()so callers can control filtering, - Or at minimum documenting that the function writes to stderr on partial failure.
Not blocking — the current behavior is fine for the CLI use case.
There was a problem hiding this comment.
Fixed in 6f6be72 — replaced print(stderr) with warnings.warn() so callers can control filtering.
| def _progress(done, total, name): | ||
| print(f"\r Downloading JS modules... {done}/{total}", end="", flush=True) | ||
|
|
||
| try: |
There was a problem hiding this comment.
Nit: the " " * 20 padding to clear the progress line works but is fragile if the progress string ever gets longer. \r + \033[K (ANSI erase-to-EOL) is more robust, or you could track the previous line length. Fine for now though.
There was a problem hiding this comment.
Fixed in 6f6be72 — using \\033[K (ANSI erase-to-EOL) instead of space padding.
|
|
||
| time.sleep(0.05) | ||
| mtimes = {} | ||
| for f in os.listdir(vendor_dir): |
There was a problem hiding this comment.
The time.sleep(0.05) makes this test sensitive to filesystem timestamp resolution. On some Linux filesystems (ext4 with 1s granularity), 50ms might not be enough. Consider 0.1 or comparing file content/hash instead of mtime. Unlikely to bite in CI but worth noting.
There was a problem hiding this comment.
Review: parallel vendor JS download with incremental caching
Verdict: Approve ✅
Clean rewrite. The BFS-style dep resolution + ThreadPoolExecutor is a solid improvement over the recursive serial approach, and the separation of download → discover → rewrite is easier to reason about than the old recursive _download_esm.
What looks good
- Concurrency model is correct.
_discover_depsmutatesurl_map/pkg_map, but only from the main thread insideas_completedloops — no concurrent mutation._openeris set once before the executor starts; reads from worker threads are safe. - BFS dep resolution guarantees complete
url_mapbefore rewriting. Import paths can only be rewritten after all transitive deps are known. The old recursive approach relied on depth-first ordering to achieve this; the new explicit two-phase (fetch-all → rewrite-all) is cleaner. - Partial failure is handled correctly. Failed downloads are collected, logged to stderr, and don't crash the process. CDN fallback is documented.
- Manifest v2 upgrade path is correct. Old v1 manifests lack
sha256, soold_files.get(name, {}).get("sha256")returnsNone→ forced re-write on first run after upgrade. - Test suite is thorough — 26 tests cover helpers, dep discovery, import rewriting, parallel download, incremental skip, and partial failure.
- CI green (lint + security).
Minor observations (non-blocking)
-
"Incremental caching" still re-fetches everything from CDN — only disk writes are skipped when hashes match. For the ~20 small ESM files this is fine, but the term "incremental" might set expectations of skipped network calls. A future enhancement could compare the local manifest first and skip
_fetchentirely for unchanged specifiers (at the cost of not detecting upstream changes). -
_rewrite_imports+_sha256computed twice per ESM file — once in the "write files" phase, once in manifest building. Storing the rewritten content would avoid the double work. Negligible perf impact at current scale, but easy to clean up. -
PDF.js binary re-read from disk for manifest — the binary data is available in memory during the
as_completedloop but discarded before manifest building, then re-read viaopen(filepath, "rb"). Same minor redundancy. -
Progress counter for PDF.js deferred — PDF.js futures are submitted first but results collected after all ESM BFS levels complete, so the progress callback won't reflect their completion until the ESM phase finishes. Cosmetic only.
None of these block merging. Nice work on the 3.6× speedup.
- Cache rewritten ESM content + hash to avoid double computation during file writing and manifest building (milo, elena) - Stash PDF.js hash during download phase, reuse in manifest instead of re-reading from disk (elena, milo) - Interleave PDF.js progress with ESM by draining completed PDF.js futures between BFS levels (milo, clementine) - Replace print(stderr) with warnings.warn() so callers can control filtering (elena, clementine) - Use ANSI erase-to-EOL (\033[K) instead of space padding for progress line clearing (elena) - Bump test sleep from 0.05s to 0.1s for ext4 mtime resolution safety (elena)
Summary
vendor.pyto useThreadPoolExecutorfor concurrent downloads (8 workers), replacing the serial recursive approachDownloading JS modules... 12/31)Benchmark: ~4s vs ~15s (3.6x speedup) on first-run vendor download.
Closes #76
Test plan
pytest tests/test_vendor.py— 26 tests passtinyleaf .in a fresh environment (delete~/.config/tinyleaf/vendor/)