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

Keep only one generation of process cache #41

Merged
merged 3 commits into from
Apr 20, 2022

Conversation

contramundum53
Copy link
Member

Originally, Kurobako caches all processes that are opened by ExternalProgramProblemFactory. This is for reducing the overhead of creating processes and loading datasets.

However, this behavior can cause a problem when large datasets are loaded in the children processes. Because Kurobako never closes processes even after those processes become unnecessary, all datasets are eventually loaded on memory and that can eat up all memory on the system.

We propose the following changes that mitigate this issue.

  • Only keep one generation of cache per thread. In other words, if the command is different from the last one, close the old process.
  • Reorder the output of kurobako studies such that the same command is executed consecutively. This is intended to maximize cache efficiency.

@coveralls
Copy link

coveralls commented Apr 20, 2022

Pull Request Test Coverage Report for Build 2193328853

  • 0 of 8 (0.0%) changed or added relevant lines in 2 files are covered.
  • 18 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.02%) to 9.545%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/study.rs 0 2 0.0%
kurobako_core/src/epi/problem/external_program.rs 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
kurobako_problems/src/nasbench.rs 1 0%
src/main.rs 1 1.61%
src/plot/curve.rs 1 0%
src/problem/rank.rs 1 0%
src/dataset.rs 2 0%
src/runner.rs 2 0%
src/dataset/surrogate.rs 5 0%
src/problem/study.rs 5 0%
Totals Coverage Status
Change from base Build 966921863: 0.02%
Covered Lines: 1101
Relevant Lines: 11535

💛 - Coveralls

Copy link
Member

@sile sile left a comment

Choose a reason for hiding this comment

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

LGTM!

@sile
Copy link
Member

sile commented Apr 20, 2022

Although CI lint checks failed, it seems not relevant to the diff by this PR. So let me fix the lint failures after merging this PR.

@sile sile merged commit 85094ea into optuna:master Apr 20, 2022
@coveralls
Copy link

coveralls commented Oct 12, 2024

Pull Request Test Coverage Report for Build 2193328853

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 8 (0.0%) changed or added relevant lines in 2 files are covered.
  • 18 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.02%) to 9.545%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/study.rs 0 2 0.0%
kurobako_core/src/epi/problem/external_program.rs 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
kurobako_problems/src/nasbench.rs 1 0.0%
src/main.rs 1 1.61%
src/plot/curve.rs 1 0.0%
src/problem/rank.rs 1 0.0%
src/dataset.rs 2 0.0%
src/runner.rs 2 0.0%
src/dataset/surrogate.rs 5 0.0%
src/problem/study.rs 5 0.0%
Totals Coverage Status
Change from base Build 966921863: 0.02%
Covered Lines: 1101
Relevant Lines: 11535

💛 - Coveralls

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.

3 participants