Skip to content

fix: output_cache.go is still using mtime instead of hashing#7

Draft
zzzz465 wants to merge 4 commits intogoforj:cmilesdev/custom-loaderfrom
zzzz465:fix/output-cache-content-hash
Draft

fix: output_cache.go is still using mtime instead of hashing#7
zzzz465 wants to merge 4 commits intogoforj:cmilesdev/custom-loaderfrom
zzzz465:fix/output-cache-content-hash

Conversation

@zzzz465
Copy link
Copy Markdown

@zzzz465 zzzz465 commented Apr 2, 2026

in previous #5 I introduced hash-based caching in discovery_cache.go and artifact_cache.go. however, I missed that I also need to patch output_cache.go which has the most impact on decreasing wire time.

this PR changes mtime based on hashing in output_cache.go, with replacing existing hashing mechanism from standard sha-256, to xx3, which appears to be more performant (note that I/O bottleneck is significant so this won't make any big difference)

also, I need to mention that hash-based equality checks naturally incur I/O, which is way slower than mtime.
in local environment, previous mtime based caching is faster than hashing, which is not possible in CI environment where mtime is always reset.

since now there's three cache (output, artifact, discovery), I did benchmark test on my private repository to see how it works with/without each cache. below is the result.

# discovery artifact output Time
1 HIT HIT HIT 1.5s
2 HIT HIT MISS 1.6s
3 HIT MISS MISS 18.6s
4 MISS HIT MISS 7.1s
5 MISS MISS MISS 21.4s

the best bet is to provide an option to choose which one to use key: mtime or hash, so user can select

@zzzz465 zzzz465 force-pushed the fix/output-cache-content-hash branch from 1fa0d8f to 06ea3a6 Compare April 2, 2026 01:26
@cmilesio
Copy link
Copy Markdown
Member

cmilesio commented Apr 2, 2026

Looks like you've got some issues on this branch, I'm going to need a bit before I can take a look at this caching issue you've ran into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants