Skip to content

Strategy: Temporarily remove response caching#10073

Merged
samford merged 1 commit intoHomebrew:masterfrom
samford:strategy-remove-response-caching
Dec 21, 2020
Merged

Strategy: Temporarily remove response caching#10073
samford merged 1 commit intoHomebrew:masterfrom
samford:strategy-remove-response-caching

Conversation

@samford
Copy link
Copy Markdown
Member

@samford samford commented Dec 20, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

This is a follow-up to #9529. I meant to address the code where header and body content for responses were being cached in @headers and @page_content hashes (using the URL as the key) but forgot along the way.

The issue as I see it is that the simple approach here caches all header or body content from responses, so memory usage continually grows over long livecheck runs (e.g., --tap homebrew/core).

Instead, we should only cache the header/body data for URLs that we know will be fetched more than once in a given run. The naive approach here has been fine for the Xorg strategy, only because the number of pages being cached is very small. A run across homebrew/core could potentially cache 2000+ pages, which is significant.

Being able to determine which URLs will be fetched more than once requires structural changes within livecheck strategies, so this will take a bit of work to implement. I've been implementing this off and on (as time permits) and I'll introduce a more sophisticated method of livecheck-wide caching in a later PR. The required changes to strategies also coincide with some refactoring that I already had planned to be able to improve the testability of livecheck code, so that work will be used regardless.

In the interim time, it's best to remove this naive caching behavior until I've finished working on an approach that provides the benefits we want (reducing duplicate fetches) while minimizing detriments (increased memory usage).

Labeling this as critical, so we can get this in ASAP.

The simple approach here caches all header or body content from
responses, so memory usage continually grows with each fetch. This
becomes more of a notable issue with long livecheck runs (e.g.,
`--tap homebrew/core`).

Instead, we should only cache the header/body for URLs that we know
will be fetched more than once in a given run. Being able to
determine which URLs will be fetched more than once requires
structural changes within livecheck strategies, so this will take a
bit of work to implement.

I've been working on this off and on and I'll introduce a more
sophisticated method of livecheck-wide caching in a later PR. In the
interim time, it's best to remove this caching behavior until I've
finished working on an approach that provides benefits (reducing
duplicate fetches) while minimizing detriments (increased memory
usage).
@samford samford added the critical Critical change which should be shipped as soon as possible. label Dec 20, 2020
@samford samford requested a review from reitermarkus December 20, 2020 18:16
@BrewTestBot
Copy link
Copy Markdown
Contributor

BrewTestBot commented Dec 20, 2020

Review period ended.

@samford samford merged commit 1eff477 into Homebrew:master Dec 21, 2020
@samford samford deleted the strategy-remove-response-caching branch December 21, 2020 06:33
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 21, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants