Switch OSD Optimizer to rely on file hashes instead of mtimes
#8472
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
❌ Changelog Entry Missing HyphenChangelog entries must begin with a hyphen (-). |
Swiddis
commented
Oct 3, 2024
| readFile$(path).pipe( | ||
| map( | ||
| (buffer) => | ||
| [path, Crypto.createHash('sha1').update(buffer).digest('base64')] as const |
There was a problem hiding this comment.
Core change is this, the rest is plumbing -- most "get the file modification ID" checks trace back to here (except one occurrence in run_compiler.ts which also was updated).
Swiddis
requested review from
ananzh,
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi and
mengweieric
as code owners
Swiddis
requested review from
LDrago27,
virajsanghvi,
sejli,
joshuali925 and
huyaboo
as code owners
AMoo-Miki
approved these changes
Oct 3, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8472 +/- ##
==========================================
- Coverage 60.94% 60.93% -0.01%
==========================================
Files 3759 3759
Lines 89329 89330 +1
Branches 13973 13973
==========================================
- Hits 54438 54437 -1
- Misses 31494 31495 +1
- Partials 3397 3398 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Codecov: this code is hit in every functional test workflow multiple times |
ruanyl
approved these changes
Oct 9, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Oct 9, 2024
* Hotswap getMtimes to compute sha1 hashes Signed-off-by: Simeon Widdis <[email protected]> * Rename all the things Signed-off-by: Simeon Widdis <[email protected]> * Changeset file for PR #8472 created/updated --------- Signed-off-by: Simeon Widdis <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 0609ecf) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
6 tasks
ruanyl
pushed a commit
that referenced
this pull request
Oct 23, 2024
… (#8532) * Hotswap getMtimes to compute sha1 hashes * Rename all the things * Changeset file for PR #8472 created/updated --------- (cherry picked from commit 0609ecf) Signed-off-by: Simeon Widdis <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Qxisylolo
pushed a commit
to Qxisylolo/OpenSearch-Dashboards
that referenced
this pull request
Oct 30, 2024
…search-project#8472) * Hotswap getMtimes to compute sha1 hashes Signed-off-by: Simeon Widdis <[email protected]> * Rename all the things Signed-off-by: Simeon Widdis <[email protected]> * Changeset file for PR opensearch-project#8472 created/updated --------- Signed-off-by: Simeon Widdis <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The current build cache in OSD optimizer depends on tracking the modification times of previous files. This means it never recognizes cached build artifacts in CI, because the artifacts get "updated" every time code is pulled. Refactoring the entire cache system to use file content hashes instead of modification times results in a minor speedup in build times, in cases where the build artifacts are cached. See #8428 for more information.
As followup, I have some discussion in opensearch-project/dashboards-observability#2188 regarding how best to actually use the cached artifacts.
Issues Resolved
Resolves #8428
Unblocks opensearch-project/dashboards-observability#2188
Screenshot
Testing the changes
Described in #8428: I set up build runs with verbose compilation enabled and tracked the built packages over time in the case of no cache, an old cache hit, and a new cache hit. Here's the results:
I also checked what breaks when pulling this on top of the old build system. Aside from a full recompilation (every file is now invalid), it doesn't seem to have issues when migrating from the old version -- if there's issues then ye olde
yarn osd cleanshould do the trick.Changelog
mtimesCheck List
yarn test:jestyarn test:jest_integration