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

Stop store content in memory for RPMBuilder #100

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ikrivosheev
Copy link
Contributor

@ikrivosheev ikrivosheev commented Mar 8, 2023

PR Content Desc

At the moment, RPMPackage store content in memory. This is not good situation. I rewrite RPMPackage to generic struct.

If my idea is ok for maintainers, I will finish the PR.

  • 🦚 Feature
  • 📙 Documentation
  • 🦣 Legacy
  • 🪣 Misc

Closes #

Changes proposed by this PR

Notes to reviewer

📜 Checklist

  • Test coverage is excellent and passes
  • Works when tests are run --all-features enabled
  • Documentation is thorough

@ikrivosheev ikrivosheev force-pushed the feature/stop_store_content_into_memory branch 2 times, most recently from 085dd33 to 6f51dfe Compare March 8, 2023 22:08
@dralley
Copy link
Collaborator

dralley commented Mar 9, 2023

Is this roughly equivalent to what #6 is describing?

Without having looked deeply at the PR yet, I think it makes sense to read the payload on-demand, but not necessarily the headers. Headers usually quite small and don't require a lot of memory to store. Doing many small reads is also less efficient than doing a small number of larger (~=8mb) reads.

@dralley
Copy link
Collaborator

dralley commented Mar 26, 2023

I'm going to try to take a step in the right direction with a PR to address #107

A general complication is that

  • you have to generate the payload in order to calculate a checksum of the payload
  • you have the calculate a checksum of the payload in order to put that checksum in the header (also other attributes like the size of the payload)
  • you have to have a completed header before you can assemble and sign the package

So the first step is just going to be to avoid storing everything twice, once independently in the list of file entries to be processed, and once in the contents of the payload.

@ikrivosheev
Copy link
Contributor Author

Well, good news) Some tests work now)

@ikrivosheev ikrivosheev force-pushed the feature/stop_store_content_into_memory branch from e064131 to 447b9e0 Compare May 23, 2023 13:49
@ikrivosheev ikrivosheev force-pushed the feature/stop_store_content_into_memory branch from 447b9e0 to fcc376c Compare June 16, 2023 18:13
@ikrivosheev ikrivosheev force-pushed the feature/stop_store_content_into_memory branch from fcc376c to c1d1735 Compare June 16, 2023 18:15
@ikrivosheev
Copy link
Contributor Author

@dralley hello! Can you review PR and say your thoughts about my PR. Does it make sense to complete it?

@dralley
Copy link
Collaborator

dralley commented Jun 17, 2023

I would hold off on continuing this for the time being. I think there are some interesting ideas here, but I'm not sure about the implementation. There are a couple of bigger problems to address first when it comes to builder memory use, and on the reading side, the introduction of RPMPackageMetadata mostly handles with that problem.

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