Skip to content

make ops structure flat #1265

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

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

make ops structure flat #1265

wants to merge 1 commit into from

Conversation

lplewa
Copy link
Contributor

@lplewa lplewa commented Apr 15, 2025

fixes: #1078

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

@lplewa lplewa requested a review from a team as a code owner April 15, 2025 13:43
@@ -28,6 +28,9 @@ extern "C" {
/// pointers.
///
typedef struct umf_memory_pool_ops_t {
/// Size of this structure.
/// Should be initialized with sizeof(umf_memory_pool_ops_t) in the current umf version
size_t size;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why version is not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As user do not know mapping between size and version. As this structure might be bigger then user is aware (as it might be extended, in future umf versions), size field is needed if user needs to copy this structure. This is why this pr adds flexible array member at the end of the struct.

Adding size field was a conclusion of the linked issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

As user do not know mapping between size and version. As this structure might be bigger then user is aware (as it might be extended, in future umf versions), size field is needed if user needs to copy this structure. This is why this pr adds flexible array member at the end of the struct.

Adding size field was a conclusion of the linked issue.

If I remember we already discussed this issue, and if I am not wrong we have not found the use case where it is needed. Could you please post a use case here where this size variable is used?

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, I am not against that, but I just do not understand why it is 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.

TBH i found this action step in final notes after discusion, in the issue, so i implemented it.

The use case is when the user want to add some extra function to our provider/pool.
To do so:

const umf_memory_pool_ops_t *ops = umfDisjointPoolOps();

umf_memory_pool_ops_t *my_ops = malloc(ops.size);
memcpy(my_ops, ops, ops.size);
my_ops.malloc = createMyCustomMallocWrapper(ops.alloc);

ofc this is also possible without size, but to do it in backward compatible way, user must reset version field (which let's be honest no one will do, and after umf update we will be blamed for compatibility break)

Copy link
Contributor

Choose a reason for hiding this comment

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

const umf_memory_pool_ops_t *ops = umfDisjointPoolOps();

umf_memory_pool_ops_t *my_ops = malloc(ops.size);
memcpy(my_ops, ops, ops.size);
my_ops.malloc = createMyCustomMallocWrapper(ops.alloc);

I think it is not a proper way to implement a wrapper on top of some memory provider. The proper way is to create a wrapper memory provider that will use a handle to another memory provider. This how our tracking provider is implemented. Another example is a tracing provider in our tests.

We should keep in mind that the proper way to use the memory provider is by creating a memory provider handle and use it via Memory Provider API, e.g. umfMemoryProviderAlloc, umfMemoryProviderFree.

The ops structures are just an API for Memory provider/pools developers provided by UMF (something like a plugin interface). The ops structures are intended to be used by UMF, not by someone else. Via ops structure we are telling to Memory provider/pools developers "if you want UMF to use your custom provider please give us the ops structure".

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.

Backward Compatibility for OPS Structures
2 participants