Closed AdrianGonz97 closed 4 months ago
Latest commit: e43d13f31e1f98e95db02af1f6f045d678117d77
The changes in this PR will be included in the next version bump.
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR
Hmm, that's really weird. I'm surprised the mtime would be different across machines. Are they different OSes? Or is the mtime set based upon when you check the repo out from git. I'd love to better understand the issue before we switch away from it
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 95.48%. Comparing base (
8790098
) to head (a4aec62
). Report is 2 commits behind head on main.:exclamation: Current head a4aec62 differs from pull request most recent head e43d13f. Consider uploading reports for the commit e43d13f to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hmm, that's really weird. I'm surprised the mtime would be different across machines. Are they different OSes?
It's definitely strange π. The OSes are set to be the same as they're running from the same workflow, so I don't think it's that. Perhaps it's more related to timezones? π€
Or is the mtime set based upon when you check the repo out from git
I'd be happy to dig into this question when I get a chance!
Or is the mtime set based upon when you check the repo out from git
It seems like this assumption was correct, @benmccann! After checking out the repo on 2 separate instances (on the same machine), I'm ending up with two different mtime
stats.
Sorry for the long delay. I'm afraid I'd forgotten about this PR.
I'm nervous about using the file size because you could hit false positives with it. I'm not sure how expensive it would be to compute a hash for every file, but it seems safer.
I had an idea that we could use mtime first and then fallback to the hash if it's different, but that probably wouldn't work because it would require us storing two hashes.
I'm nervous about using the file size because you could hit false positives with it.
Fair enough!
I'm not sure how expensive it would be to compute a hash for every file, but it seems safer.
I agree. Testing locally, I modified it to use imageBuffer
instead, which had no perceivable performance impact relative to the rest of build.
I also did some microbenchmarking for it (just in case), testing different image sizes to see their impact:
βββββββββββ¬ββββββββββββββββββββββββββββββββββββ¬ββββββββββββ¬βββββββββββββββββββββ¬βββββββββββ¬ββββββββββ
β (index) β Task Name β ops/sec β Average Time (ns) β Margin β Samples β
βββββββββββΌββββββββββββββββββββββββββββββββββββΌββββββββββββΌβββββββββββββββββββββΌβββββββββββΌββββββββββ€
β 0 β 'hash with size - base (500kb)' β '231,842' β 4313.26988522398 β 'Β±0.63%' β 231843 β
β 1 β 'hash with buffer - base (500kb)' β '5,979' β 167226.18963211094 β 'Β±0.23%' β 5980 β
β 2 β 'hash with buffer - 11mb' β '165' β 6036291.090361429 β 'Β±0.43%' β 166 β
β 3 β 'hash with buffer - 5mb' β '311' β 3215181.397435922 β 'Β±0.31%' β 312 β
β 4 β 'hash with buffer - 2.5mb' β '1,099' β 909751.2799999929 β 'Β±0.20%' β 1100 β
β 5 β 'hash with buffer - 1mb' β '2,599' β 384681.17346155015 β 'Β±0.40%' β 2600 β
β 6 β 'hash with buffer - 500kb' β '5,875' β 170197.13359428805 β 'Β±0.28%' β 5876 β
β 7 β 'hash with buffer - 350kb' β '6,425' β 155626.61889200658 β 'Β±0.25%' β 6426 β
β 8 β 'hash with buffer - 100kb' β '16,379' β 61051.895238088415 β 'Β±0.44%' β 16380 β
βββββββββββ΄ββββββββββββββββββββββββββββββββββββ΄ββββββββββββ΄βββββββββββββββββββββ΄βββββββββββ΄ββββββββββ
Here's the repo for it too: https://github.com/AdrianGonz97/imagetools-hashing-benchmarks
Hashing with the size
(or mtime
) stat is significantly faster, however, I would consider hashing with the image to be sufficiently fast enough for our needs. Especially since this operation doesn't execute hundreds of thousands of times per build.
So in practice, I think it's completely fine to hash with the image itself (even without the mtime
optimization you described) as the performance impact is miniscule when compared to the rest of the build.
Side note: While writing the benchmark, I noticed that generateImageID
is async when it doesn't need to be. Do you have any idea why that's the case? Scratch that, looking back at the history, it seems like it used to await a promise before the caching PR was introduced. Not a problem, I can fix that.
This PR fixes an issue that we've been experiencing in https://github.com/huntabyte/shadcn-svelte/pull/978 where the hash for the generated image ID is different when it's computed on different machines. The goal of that PR was to cache the images so that they can be utilized across our GH runners.
The source of the issue is the
mtime
stat from the image file, which seemingly differs from machine to machine, causing the hash to generate a different id, resulting in a cache MISS. Instead,mtime
has been replaced for thesize
stat, which (in conjunction with the image config and file url) should provide sufficient uniqueness for the hash.Quick Checklist
[x] I have read the contributing guidelines
[x] I have written new tests, as applicable (for bug fixes / features)
[ ] Docs have been added / updated (for bug fixes / features)
[x] I have added a changeset, if applicable
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) Bug fix
What is the new behavior (if this is a feature change)? Implements consistent image ID hashing across machines
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) No
Other information: I wasn't sure the best way to express this change in a test, so I've hardcoded the expected id string. I'm sure there's a better way to do it, so please feel free to modify it!