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

feat: auto config cache size according to memory size #3165

Merged
merged 9 commits into from Jan 17, 2024

Conversation

QuenKar
Copy link
Contributor

@QuenKar QuenKar commented Jan 15, 2024

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

  • according to host memory size to decide the mito's buffer or cache size.
  • remove cpu_nums crate and use sysinfo.

For example, my computer has 16G memory and 8 cpu cores


Mito(
    MitoConfig {
        num_workers: 4,
        worker_channel_size: 128,
        worker_request_batch_size: 64,
        manifest_checkpoint_distance: 10,
        compress_manifest: false,
        max_background_jobs: 4,
        auto_flush_interval: 1800s,
        global_write_buffer_size: 256.0MiB,
        global_write_buffer_reject_size: 512.0MiB,
        sst_meta_cache_size: 64.0MiB,
        vector_cache_size: 128.0MiB,
        page_cache_size: 128.0MiB,
        enable_experimental_write_cache: false,
        experimental_write_cache_path: "",
        experimental_write_cache_size: 128.0MiB,
        sst_write_buffer_size: 8.0MiB,
        scan_parallelism: 2,
        parallel_scan_channel_size: 32,
        allow_stale_entries: false,
    ...
    ...
    },
),

The possible concern here is sysinfo couldn't get system info in some unpopular OS.


update 2024.1.17
docker container(ubuntu 24.04), host memory=28G, cpus=16, we limit cpus = 8, memory = 8G, test again.
the result:

MitoConfig {
    num_workers: 4,
    worker_channel_size: 128,
    worker_request_batch_size: 64,
    manifest_checkpoint_distance: 10,
    compress_manifest: false,
    max_background_jobs: 4,
    auto_flush_interval: 1800s,
    global_write_buffer_size: 128.0MiB,
    global_write_buffer_reject_size: 256.0MiB,
    sst_meta_cache_size: 32.0MiB,
    vector_cache_size: 64.0MiB,
    page_cache_size: 64.0MiB,
    enable_experimental_write_cache: false,
    experimental_write_cache_path: "",
    experimental_write_cache_size: 64.0MiB,
    sst_write_buffer_size: 8.0MiB,
    scan_parallelism: 2,
    parallel_scan_channel_size: 32,
    allow_stale_entries: false,
    ...
    ...
},

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Refer to a related PR or issue link (optional)

@github-actions github-actions bot added docs-not-required This change does not impact docs. Size: S labels Jan 15, 2024
src/mito2/src/config.rs Outdated Show resolved Hide resolved
@QuenKar QuenKar marked this pull request as ready for review January 16, 2024 03:55
@QuenKar QuenKar marked this pull request as draft January 16, 2024 06:10
@github-actions github-actions bot added Size: M and removed Size: S labels Jan 16, 2024
@github-actions github-actions bot added Size: S and removed Size: M labels Jan 17, 2024
@QuenKar QuenKar marked this pull request as ready for review January 17, 2024 06:11
@github-actions github-actions bot added Size: M and removed Size: S labels Jan 17, 2024
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c2edaff) 85.63% compared to head (24603b8) 85.31%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3165      +/-   ##
==========================================
- Coverage   85.63%   85.31%   -0.32%     
==========================================
  Files         829      829              
  Lines      135773   136163     +390     
==========================================
- Hits       116273   116172     -101     
- Misses      19500    19991     +491     

@QuenKar
Copy link
Contributor Author

QuenKar commented Jan 17, 2024

@zhongzc I add the check about cgroup, if you have time, PTAL.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/mito2/src/config.rs Show resolved Hide resolved
src/mito2/src/config.rs Outdated Show resolved Hide resolved
src/common/config/src/utils.rs Outdated Show resolved Hide resolved
src/mito2/src/config.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
QuenKar and others added 2 commits January 17, 2024 16:33
Co-authored-by: Yingwen <realevenyag@gmail.com>
Co-authored-by: Dennis Zhuang <killme2008@gmail.com>
@evenyag evenyag added this pull request to the merge queue Jan 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2024
@killme2008 killme2008 added this pull request to the merge queue Jan 17, 2024
Merged via the queue into GreptimeTeam:main with commit 3d7d2fd Jan 17, 2024
15 checks passed

/// Get the total memory of the system.
/// If `cgroup_limits` is enabled, it will also check it.
pub fn get_sys_total_memory() -> Option<ReadableSize> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have an idea:
Can we obtain a logical memory size here, which should be smaller than the system memory.
Consider the following scenario:
The user deploys our DB on a machine with a large amount of memory, and this machine is also running other processes.
They want our DB to occupy less memory. It would be better if we can provide a parameter to specify the memory size, for example, by specifying it through startup parameters.
Then in the get_sys_total_memory method, we return the minimum value between it and sys_total_memory: Min(logical_total_memory, sys_total_memory).

The same idea for the get_cpus()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants