Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: apt cache performance #104

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

stevenh
Copy link
Contributor

@stevenh stevenh commented May 2, 2023

Use a single call to apt-cache to reduce the time needed to lookup package versions.

Also:

  • Added millisecond details to log timing so slow operations can be more easily identified.
  • Perform apt update before determining package versions.

Fixes #103

Use a single call to apt-cache to reduce the time needed to lookup
package versions.

Also:
* Added millisecond details to log timing so slow operations can be more
  easily identified.
* Perform apt update before determining package versions.

Fixes awalsh128#103
@stevenh stevenh marked this pull request as ready for review May 2, 2023 21:57
@stevenh
Copy link
Contributor Author

stevenh commented May 2, 2023

In a simple test case with 7 packages this reduced the processing time from 17s to 4s for the cached case.

This makes it quicker to use this action than to install the packages natively, which previously wasn't the case.

Copy link
Owner

@awalsh128 awalsh128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this PR. Added some minor feedback.

Can you redo the PR into the dev branch? That way I can run it against the CI.

lib.sh Outdated Show resolved Hide resolved
lib.sh Outdated Show resolved Hide resolved
lib.sh Show resolved Hide resolved
lib.sh Outdated Show resolved Hide resolved
lib.sh Outdated Show resolved Hide resolved
lib.sh Outdated Show resolved Hide resolved
lib.sh Outdated Show resolved Hide resolved
Added the review feedback, updating variable names to be more
descriptive and using log_err where appropriate.
@stevenh
Copy link
Contributor Author

stevenh commented May 5, 2023

Thanks for the review @awalsh128 I believe I've addressed the feedback.

If you're happy with how it now looks I can do PR against develop, just wanted to update this one first to make sure I catch all the feedback.

@stevenh
Copy link
Contributor Author

stevenh commented May 25, 2023

Quick nudge to see if we can get this one in @awalsh128

@bonlime
Copy link

bonlime commented Jul 3, 2023

@awalsh128 what stops this from being merged?

@richarddd
Copy link

Any update?

@awalsh128 awalsh128 merged commit 641f947 into awalsh128:master Oct 11, 2023
@awalsh128
Copy link
Owner

Sorry for the really long wait. It has now been merged into master. I'll do a release today or tomorrow.

@stevenh stevenh deleted the fix/apt-cache-performance branch October 11, 2023 15:20
@awalsh128
Copy link
Owner

This is now included in release v1.3.1.

Although it looks like it is causing 2 regressions on the CI.

The most critical is the multiline block one. If I can't resolve in a couple of days I will need to pull the change for now. Could you help diagnose the issue introduced with the latest change (verified it is in this PR)?

@stevenh
Copy link
Contributor Author

stevenh commented Oct 30, 2023

This is now included in release v1.3.1.

Although it looks like it is causing 2 regressions on the CI.

The most critical is the multiline block one. If I can't resolve in a couple of days I will need to pull the change for now. Could you help diagnose the issue introduced with the latest change (verified it is in this PR)?

Could you describe how I can validate this, so I can investigate @awalsh128 ?

@awalsh128
Copy link
Owner

The easiest to tackle is https://github.com/awalsh128/cache-apt-pkgs-action-ci/actions/runs/6697244906/job/18196712208 which tests that two package lists of different order should be the same in terms of a cache hit.

I think the issue here is that the returned normalized package list no longer sorts. Below is the difference now between the pre-hashed value.

xdot=1.2-2 rolldice=1.16-1build1 @ 6697244906-1-standard_workflow 1
rolldice=1.16-1build1 xdot=1.2-2 @ 6697244906-1-standard_workflow 1

BTW, received a report today where it is failing on the function. #116
Isn't open source fun? 😆

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.

Verifying packages is slow
4 participants