c-pal code changes for gballoc_hl/ll_set_option #409
Conversation
microsoft-github-policy-service
bot
requested review from
anporumb,
avranju,
mattdurak,
dcristoloveanu,
jebrando,
darobs,
ipg2013 and
parth21999
RohitG28
force-pushed
the
jemalloc_decay
branch
from
2c2c3e3
to
501e692
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
win32/devdoc/gballoc_ll_jemalloc.md
Outdated
| ``` | ||
|
|
||
| `gballoc_ll_set_decay` sets the dirty and muzzy pages decay time to `decay_milliseconds` milliseconds. |
There was a problem hiding this comment.
sets the dirty and muzzy pages decay time to
decay_millisecondsmilliseconds.
One would argue that decay_milliseconds should be
- part of "runtime" API (and that happens here)
- part of "init" API (gballoc_ll_init takes a void* and this value should be behind that void*).
Because what happens with all the allocates between
gballoc_ll_init and
gballoc_ll_set_decay?
If decay_milliseconds exists at the time of gballoc_ll_init then it should be used by gballoc_ll_init.
There was a problem hiding this comment.
The allocations in between would happen as is but once the decay values are set they will just be held for longer in the dirty and muzzy pool rather than being released immediately. So decay values can actually be set before init as well since its an arena level parameter. However, if we plan to pass the decay value to gballoc_hl/ll_init, do you want me to have the same value passed to both dirty and muzzy decay options? We have now separated both the calls. Or should we stick to calling the set_option later?
| int64_t verify_decay_ms; | ||
| size_t decay_ms_size = sizeof(decay_ms); | ||
|
|
||
| // Arena 1 does not exist, so je_mallctl should return EFAULT |
There was a problem hiding this comment.
no the test deliberately skips creating this arena 1. Huge arena is the last arena(arena 4)
| { | ||
| gballoc_ll_free(ptr[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we're missing tests here, and it is better to have them now and here (in c-pal) rather than to discover that they don't work later and in production.
We should have a test that actually sees when memory is released to OS and depending on the config, it is released earlier/later/never at all.
I can imagine something like:
let's start like with a number of threads, all of them pretending to do memory work (alloc/free) for an amount of time.
Following this amount of time, some memory should still be used, some memory should be free(). Take a snapshot of the memory used by the process.
Wait past the decay timer (there's no other activity)
Take another snapshot of the memory used by the process.
The diff between the 2 snapshots and the decay parameter should be correlated as a condition to pass the test. With some tolerances, ofc.
There was a problem hiding this comment.
I still think this PR requires at least the above work to be completed, so I'll still be waiting. Rest looks good.
There was a problem hiding this comment.
I have added new tests that check the dirty and muzzy page numbers after we decay the arenas. Hope these would be sufficient to test the changes?
There was a problem hiding this comment.
I looked at the tests, and while they do seem to test what I've mentioned here, it is a case of "testing jemalloc with jemalloc".
When I said before "Take a snapshot of the memory used by the process." I was referring to the working set size. So let's compare the working set "before" and "after" by calling Windows APIs and not use jemalloc's internals.
There was a problem hiding this comment.
Need to see the next version of the INT tests with Windows.
I understand if there are no INT tests for Linux at this time, but there should be some INT tests for Linux too using the parallel mechanisms existing for Linux.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| int64_t low_dirty_decay_ms = 1; | ||
| result = gballoc_ll_set_option("dirty_decay", &low_dirty_decay_ms); | ||
| ASSERT_ARE_EQUAL(int, 0, result); | ||
| result = gballoc_ll_set_option("muzzy_decay", &decay_ms); |
There was a problem hiding this comment.
probably line 439 needs to become line 437.5
| if (je_mallctl("arenas.dirty_decay_ms", &old_decay_milliseconds, &old_decay_milliseconds_size, &decay_milliseconds, sizeof(decay_milliseconds)) != 0) | ||
| { | ||
| /*Codes_SRS_GBALLOC_LL_JEMALLOC_28_018: [ If there are any errors, gballoc_ll_set_option shall fail and return a non-zero value. ]*/ | ||
| LogError("je_mallctl(arenas.dirty_decay_ms, &old_decay_milliseconds=%p, &old_decay_milliseconds_size=%p, &decay_milliseconds=%p, sizeof(decay_milliseconds)=%zu) failed", &old_decay_milliseconds, &old_decay_milliseconds_size, &decay_milliseconds, sizeof(decay_milliseconds)); |
| if (je_mallctl("arenas.muzzy_decay_ms", &old_decay_milliseconds, &old_decay_milliseconds_size, &decay_milliseconds, sizeof(decay_milliseconds)) != 0) | ||
| { | ||
| /*Codes_SRS_GBALLOC_LL_JEMALLOC_28_018: [ If there are any errors, gballoc_ll_set_option shall fail and return a non-zero value. ]*/ | ||
| LogError("je_mallctl(arenas.muzzy_decay_ms, ol&old_decay_millisecondsdp=%p, &old_decay_milliseconds_size=%p, &decay_milliseconds=%p, sizeof(decay_milliseconds)=%zu) failed", &old_decay_milliseconds, &old_decay_milliseconds_size, &decay_milliseconds, sizeof(decay_milliseconds)); |
| return result; | ||
| } | ||
|
|
||
| static int gballoc_ll_set_muzzy_decay(int64_t decay_milliseconds) |
No description provided.