Skip to content

Split initialize function and add post-initialize phase #1467

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Split initialize function and add post-initialize phase #1467

wants to merge 1 commit into from

Conversation

KFilipek
Copy link
Contributor

Description

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used
  • All API changes are reflected in docs and def/map files, and are tested

@KFilipek KFilipek self-assigned this Jul 25, 2025
@KFilipek KFilipek requested a review from a team as a code owner July 25, 2025 09:25
@bratpiorka bratpiorka marked this pull request as draft July 28, 2025 09:22
@KFilipek KFilipek force-pushed the post_initialize branch 17 times, most recently from 54df467 to 40c6dfc Compare August 1, 2025 08:46
@KFilipek KFilipek marked this pull request as ready for review August 1, 2025 10:14
@KFilipek KFilipek requested review from lplewa and bratpiorka August 1, 2025 10:15
@KFilipek
add post-initialize function to pools and providers
1f76309 
Split between initialize and post-initialize function is necessary
for properly handling CTL defaults.
lplewa
lplewa requested changes Aug 1, 2025
View reviewed changes
include/umf/memory_pool_ops.h
Comment on lines +171 to +176
/// @param provider memory provider that will be used for coarse-grain allocations.
/// Should contain at least one memory provider.
/// @param numProvider number of elements in the providers array
/// @param params pool-specific params, or NULL for defaults
/// @param pool [out] returns pointer to the pool
/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Write better documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/memory_provider.c
@@ -250,6 +276,16 @@ umf_result_t umfMemoryProviderCreate(const umf_memory_provider_ops_t *ops,

provider->provider_priv = provider_priv;

if (provider->ops.ext_post_initialize != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this if is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/pool/pool_disjoint.c Outdated
Comment on lines 38 to 53
static umf_result_t CTL_READ_HANDLER(name)(void *ctx,
umf_ctl_query_source_t source,
void *arg, size_t size,
umf_ctl_index_utlist_t *indexes) {
static umf_result_t CTL_READ_HANDLER(name)(void* ctx,
umf_ctl_query_source_t source,
void* arg, size_t size,
umf_ctl_index_utlist_t* indexes) {
(void)source, (void)indexes;

disjoint_pool_t *pool = (disjoint_pool_t *)ctx;
disjoint_pool_t* pool = (disjoint_pool_t*)ctx;

if (arg == NULL) {
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

if (size > 0) {
strncpy((char *)arg, pool->params.name, size - 1);
((char *)arg)[size - 1] = '\0';
strncpy((char*)arg, pool->params.name, size - 1);
((char*)arg)[size - 1] = '\0';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wtf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected. Funny fact is that this is ignored by code check, I suppose that #if 0 is here a perpetrator.

src/pool/pool_disjoint.c Outdated
(void)source, (void)indexes, (void)size;
disjoint_pool_t *pool = (disjoint_pool_t *)ctx;
Copy link
Contributor

Choose a reason for hiding this comment

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

dito

src/memory_pool.c
@@ -261,7 +276,16 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,
}
}

if (ops->ext_post_initialize != NULL) {
ret = ops->ext_post_initialize(pool->provider, params, pool->pool_priv);
Copy link
Contributor

Choose a reason for hiding this comment

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

document what happens if post initialize fails. Who is responsible for clearing things initialized in initialize bunction

src/pool/pool_disjoint.c
Comment on lines -841 to -844

err_free_disjoint_pool:
umf_ba_global_free(disjoint_pool);

Copy link
Contributor

Choose a reason for hiding this comment

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

is it memleak? Who is respoinsible for this free?

lplewa
lplewa reviewed Aug 1, 2025
View reviewed changes
test/pools/jemalloc_pool.cpp
@@ -161,7 +161,7 @@ TEST_F(test, jemallocPoolParams) {
EXPECT_EQ(ret, UMF_RESULT_SUCCESS);
}

TEST_F(test, jemallocPoolParamsInvalid) {
TEST_F(test, jemallocPoolParamsInvalid) { // TODO FIX THIS
Copy link
Contributor

Choose a reason for hiding this comment

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

????

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lplewa lplewa lplewa requested changes

@bratpiorka bratpiorka Awaiting requested review from bratpiorka

Requested changes must be addressed to merge this pull request.

Assignees

@KFilipek KFilipek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KFilipek @lplewa