Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

feat: support to set/get dirty_decay_ms and muzzy_decay_ms for jemalloc #928

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

empiredan
Copy link
Contributor

@empiredan empiredan commented Sep 29, 2021

In jemalloc, there are 2 important options for the two-phase, decay-based purging, with each option corresponding to a phase. They are:

And this PR will provide 2 remote commands to set/get each option dynamically even if rDSN built with jemalloc is running. The 2 commands's format are as below:

replica.set-jemalloc-arena-dirty-decay-ms <arena_index | ALL> [decay_ms | DEFAULT | DISABLE]
replica.set-jemalloc-arena-muzzy-decay-ms <arena_index | ALL> [decay_ms | DEFAULT | DISABLE]

Actually, the arguments of the 2 commands are exactly the same.

The 1st argument is required. A valid value for it should be a arena_index or ALL. Suppose that the number of arenas is n, then the range of arena_index is [0, n). ALL means setting/getting for all arenas.

The 2nd argument is optional. If it is missing, the value of dirty_decay_ms or muzzy_decay_ms will be returned, which means getting (rather than setting) a value for any arena or the values for all arenas.

If the 2nd argument is given, a valid value for it should be a decay_ms, DEFAULT, or DISABLE. Just as its name implies, decay_ms means the numerical value of dirty_decay_ms or muzzy_decay_ms, whose unit is milliseconds. The value is valid only if it's equal or more than 0.
Or, it should be either of the following values:

  • DEFAULT: for dirty_decay_ms, the value is 10000, namely 10 seconds; for muzzy_decay_ms, the value is 0, which means immediately purging once dirty/muzzy pages are created.
  • DISABLE: for both dirty_decay_ms and muzzy_decay_ms, the value is -1, which means disabling purging.

// specific language governing permissions and limitations
// under the License.

#ifdef DSN_USE_JEMALLOC
Copy link
Member

Choose a reason for hiding this comment

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

DSN_USE_JEMALLOC is a macro, it's definite in cmake, so we can skip compile je_ctl.cpp, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's right.

Here I've referenced the similar implementation of DSN_ENABLE_GPERF as defined in bin/dsn.cmake:

    if(ENABLE_GPERF)
        set(DSN_SYSTEM_LIBS ${DSN_SYSTEM_LIBS} tcmalloc_and_profiler)
        add_definitions(-DDSN_ENABLE_GPERF)
    endif()

    if(USE_JEMALLOC)
        find_package(Jemalloc REQUIRED)
        # also use cpu profiler provided by gperftools
        set(DSN_SYSTEM_LIBS ${DSN_SYSTEM_LIBS} JeMalloc::JeMalloc profiler)
        add_definitions(-DDSN_USE_JEMALLOC)
    endif()

Once ENABLE_GPERF is opened, DSN_ENABLE_GPERF will be defined. Accordingly, #ifdef DSN_ENABLE_GPERF is placed on the header of src/http/pprof_http_service.h, meaning that the tools for gperf won't be compiled as long as it is disabled.

@Smityz
Copy link
Contributor

Smityz commented Oct 25, 2021

Is it possible to add some function/unit tests? It's a big PR, and I am not sure its correctness

@empiredan
Copy link
Contributor Author

Is it possible to add some function/unit tests? It's a big PR, and I am not sure its correctness

@Smityz Yes, actually as is discussed with @acelyc111, I also worry about this problem, even jemalloc related code will not be compiled for each commit since now the macro USE_JEMALLOC is disabled. I'll add some unit tests.

@acelyc111
Copy link
Member

Is it possible to add some function/unit tests? It's a big PR, and I am not sure its correctness

@Smityz Yes, actually as is discussed with @acelyc111, I also worry about this problem, even jemalloc related code will not be compiled for each commit since now the macro USE_JEMALLOC is disabled. I'll add some unit tests.

I think a better way to check jemalloc is to do smoke test, for example, add a jemalloc test section in .github/workflows/pull_request.yaml, i.e. enable USE_JEMALLOC, and run all unit tests.

@Smityz
Copy link
Contributor

Smityz commented Dec 16, 2021

Is it possible to add some function/unit tests? It's a big PR, and I am not sure its correctness

@Smityz Yes, actually as is discussed with @acelyc111, I also worry about this problem, even jemalloc related code will not be compiled for each commit since now the macro USE_JEMALLOC is disabled. I'll add some unit tests.

I think a better way to check jemalloc is to do smoke test, for example, add a jemalloc test section in .github/workflows/pull_request.yaml, i.e. enable USE_JEMALLOC, and run all unit tests.

Yes, we can add a new workflow

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants