Backward Compatibility for OPS Structures #1078
Comments
|
I personal vote for option 3. |
|
I need to take a look at #1063, but there isn't really any issue of just using the version variable as a sort of size (like in your option 3). Basically I think this should work like this:
Let's not overcomplicate this... |
|
@lplewa in fact in #1063 I implemented a mix of 2a + b - lets call it just "2":
Advantages:
Disadvantages:
Now if we compare option 2 with 3:
(2) User-Defined Custom Structure
(3) Duplicating UMF’s OPS Structure
(4) Old code (compiled against old UMF) linking with new UMF
Complexity in library maintenance
Ability to add fields
Ability to remove/rename fields
Risk of user errors
Additionally: break changes with the current release:
From what I see option 2 wins with 3 in all fields except the dlsym case where option 2 could easily support only single version back @pbalcer let's not oversimplify this :) this whole thread is about this single sentence "This requires we bump up the version (e.g., from 0.11.3 to 0.11.4)" - the "version" here could be an int or size of structure, and we are trying to figure out here which option is the best |
I don't think it matters. We can map both numbers to "can I use this field or not". The only consideration might be how simple it is to do that mapping. Size might be easier, whereas version might require a switch like I suggested. Personally I have no preference, both will work. I don't understand the need to keep other versions of the structs, returning older versions etc. |
|
@pbalcer ops structures are not opaque - the user sees their definition and what about: ops arr[3]; // sizeof(ops) == 32 I do not understand why we could use the size as a version. Size is size, version is version. Whatever we use, there need to be a way to get the "version" from the ops structure. Consider: ops { // ver 0.10 from the included file umfProviderCreate(my_ops); In the example above umfProviderCreate needs to recognize the version of my_ops to properly handle .b. |
|
Copying structs like this will never just work. Not with size or version. It needs to be updated after such a copy. The answer here is documentation.
You can map version to size. Again, I don't care either way.
Deprecating features is something that will need to be handled case-by-case. I don't understand how we could handle it generically. |
Ok - my bad - i did not see macros
We need good documentation - if used do anything with ops structure must look on the docs in header, so it should be fine.
Only if you do not rename, or remove field from the struct
You cannot change struct anyway - you can add only optional fields in both cases.
Nope - you cannot remove fields in option two anyway. See cause (4)
We broke it anyway by moving free from ext to main struct.
Thinks that win in option 2 are things you cannot do anyway do to requirement (4), as this is public structure, so we cannot remove or rename fields. User program must always compile, |
We need size - user might get osprovider ops, duplicate them, and then i.e change one function to his (i.e to implement some extra logging). to duplicate struct user must know size of the struct, as it might be larger then this what he got from his header. |
The possible scenario is something like this: Not much difference either way. We can even add a convince macro/function for this specific purpose: Again, I don't mind either solution. If you all think size is better, we can do size. The only consideration is that we are breaking the existing structs. But that might be a worthwhile compromise... |
|
I agree with @pbalcer that it does not matter
I think bullets 1 - 3 are possible scenarios and bullet 4 is the only subject for discussion regarding backward compatibility (in the abovementioned scenarios). If versions of UMF at compile-time and runtime are the same there are no issues with the abovementioned scenarios 1 - 3. Scenario 1 is not an issue, since runtime returns the pointer to the Scenario 2 is not an issue as well, because the version of the UMF at runtime should be higher or equal to the version of UMF used by the client at compile-time. And UMF should know how to handle older versions of Scenario 3: // byte copying
umf_memory_pool_ops_t *src_ops = umfDisjointPoolOps();
umf_memory_pool_ops_t dst_ops;
memcpy(&dst_ops, src_ops, sizeof(umf_memory_pool_ops_t ));
// do not forget to set the right version in dst_ops.
// Otherwise, it will contain the version from src_ops
dst_ops.version = UMF_VERSION_CURRENT;
// field-copying
dst_ops.version = UMF_VERSION_CURRENT;
dst_ops.initialize = src_ops->initialize;
dst_ops.finalize = src_ops->finalize;
...In case the In case of the version of UMF used at run-time is less than the version of UMF used to compile the client code, it is a forward compatibility scenario that we are not going to support. |
|
There is multiple options: const umf_memory_provider_ops_t * ops = umfOsMemoryProviderOps(void);
umf_memory_provider_ops_t *myops = malloc(ops.size);
memcpy(myops, ops, ops.size);
myops.free = &my_custom_free_function;
// this code is wrong
//umf_memory_provider_ops_t *myops = malloc(sizeof(*myops));
//memcpy(myops, ops, sizeof(*myops));
// this is fine
umf_memory_provider_ops_t *myops = malloc(sizeof(*myops));
//or
umf_memory_provider_ops_t o , *myops = &o;
myops->version = UMF_VERSION_CURRENT; //or myops->size= sizeof(*myops)@bratpiorka's solution solves this pitfall for the user by providing different version umfOsMemoryProviderOps(), for each change in ops structure. So always struct returned has the same size, that user compiled against their program. In his case user can just do. const umf_memory_provider_ops_t * ops = umfOsMemoryProviderOps(void);
umf_memory_provider_ops_t *myops = malloc(sizeof(*myops));
memcpy(myops, ops, sizeof(*myops));
assert(myops.size == sizeof(*myops); // allways true in this approach.But this solution comes at cost extra complication in case of dlsym() usage, and extra maintenance burden in umf code. |
|
First, I would prefer the Second, for me, this looks like a hacky error-prone approach: umf_memory_provider_ops_t *myops = malloc(ops.size);
memcpy(myops, ops, ops.size);
myops.free = &my_custom_free_function; We might think of providing API to create a copy of ops so that users can modify the copy in-place. But how important is such a use case? I would not say this example is wrong: // this code is wrong
//umf_memory_provider_ops_t *myops = malloc(sizeof(*myops));
//memcpy(myops, ops, sizeof(*myops));It is similar to the "byte copying" example I provided above.
That is why we should not overcomplicate the solution to cover "fancy" scenarios. It is good that we are discussing it here, but the solution should take into account which scenario we want to handle and which we don't. |
|
100% agreed with @vinser52 |
|
Great to see everyone agreed :) so either please review my PR, or propose a PR with a solution + list what case it covers. Please note that we plan to clean up the API + add CTL-related functions to the ops and ideally we do not want to break the API in either case. |
This is separate issue - please see: #1080
You must do this, or reset version/size field after memcpy. This is also error-prone.
You must reset size/version field. Otherwise if user compile agains older umf version and then uses it with new one.
I agree. Just wanted to explain it's use case if the other option. |
Agree that it is error-prone as well. Furthermore, now I am thinking that even copying the ops structure and modifying a particular callback is error-prone because it breaks encapsulation. For example, to substitute
In general I think that we (UMF developers) should avoid delegation of compatibility issues to the customers if possible. It is our (not the user's) responsibility to provide backward compatibility.
yeah, fully agree that we need to discuss such questions to make weighted decisions. |
This is why i think size > version but this is not a huge deal |
|
Let me summarize discussion,
typedef struct umf_memory_provider_ext_ops_t {
umf_result_t (*purge_lazy)(void *provider, void *ptr, size_t size);
umf_result_t (*purge_force)(void *provider, void *ptr, size_t size);
umf_result_t (*allocation_merge)(void *hProvider, void *lowPtr,
void *highPtr, size_t totalSize);
umf_result_t (*allocation_split)(void *hProvider, void *ptr,
size_t totalSize, size_t firstSize);
} umf_memory_provider_ext_ops_t;
typedef struct umf_memory_provider_ipc_ops_t {
umf_result_t (*get_ipc_handle_size)(void *provider, size_t *size);
umf_result_t (*get_ipc_handle)(void *provider, const void *ptr, size_t size,
void *providerIpcData);
umf_result_t (*put_ipc_handle)(void *provider, void *providerIpcData);
umf_result_t (*open_ipc_handle)(void *provider, void *providerIpcData,
void **ptr);
umf_result_t (*close_ipc_handle)(void *provider, void *ptr, size_t size);
} umf_memory_provider_ipc_ops_t;
typedef struct umf_memory_provider_ops_t {
uint32_t version;
umf_result_t (*initialize)(void *params, void **provider);
void (*finalize)(void *provider);
umf_result_t (*alloc)(void *provider, size_t size, size_t alignment,
void **ptr);
umf_result_t (*free)(void *provider, void *ptr, size_t size);
void (*get_last_native_error)(void *provider, const char **ppMessage,
int32_t *pError);
umf_result_t (*get_recommended_page_size)(void *provider, size_t size,
size_t *pageSize);
umf_result_t (*get_min_page_size)(void *provider, void *ptr,
size_t *pageSize);
const char *(*get_name)(void *provider);
umf_memory_provider_ext_ops_t ext;
umf_memory_provider_ipc_ops_t ipc;
} umf_memory_provider_ops_t; |
|
ok, so here is my summary/proposal:
draft: #1087 |
Our OPS structures for pools and providers are currently not backward compatible, even though they include a
versionfield. The following are four separate requirements we must fulfill:User calls
umfDisjointPoolOps()and passes the resulting structure toumf*Create().(No manual struct creation—just calling the UMF-provided function.)
User creates their own provider/pool structure (manually defining the fields) and passes it to
umf*Create().(We need to detect the old structure version and handle it properly.)
User obtains our OPS structure, duplicates it (it SHOULD be const), and modifies it.
(Challenge: if the new UMF version’s OPS structure has a different size, the user doesn’t know how much memory to allocate.)
User compiles a program (written against an older UMF release) without any source changes, with a new UMF version.
(The program should compile without problems and continue to work.)
Option 1: Make OPS Structure Opaque
Concept
umfPoolOpsCreate(),umfPoolOpsDup(),umfPoolOpsDestroy()).Advantages
Disadvantages
Option 2: Keep Old Versions Internally
We maintain multiple versions of the OPS structure and the functions that return them. That way, older user code can call older “versions” and still work. There are two sub-approaches:
Option 2a: Symbol Versioning (Linker Script)
Concept
umfDisjointPoolOpsmight exist in multiple versions:_umfDisjointPoolOps@UMF_1.0,_umfDisjointPoolOps@UMF_2.0, etc.Advantages
Disadvantages
dlsymchanges will break compatibility unless the user usesdlvsym.Option 2b: Version-Suffixed Functions + Macro
Concept
umfDisjointPoolOpsV1(),umfDisjointPoolOpsV2(), etc.#define umfDisjointPoolOps(...) umfDisjointPoolOpsV2(...)points to the newest version.Advantages
umfDisjointPoolOpsV1()(or if the macro forumfDisjointPoolOpspoints toV1initially).Disadvantages
umfDisjointPoolOpsV2()or whichever is current.Option 3: Replace
versionField with Structure SizeConcept
versionfield and replace it with asizefield.ops->sizeto know how many bytes to copy.ext,ipc) from the provider struct and place everything in one contiguous ops structure. This prevents layout fragmentation when new fields are inserted.Advantages
size.versionto setting asize, for custom pools/providersDisadvantages
versionis removed and replaced bysize.sizeofor assume a fixed struct size instead of using thesizefield.Comparison Table
(Opaque Structure)
(Symbol Versioning)
(Version-Suffixed + Macro)
(Structure Size)
umfDisjointPoolOps()and passing toumfCreate()umfDisjointPoolOpsV1()(or macro forV1)ops->size; if it’s smaller, treat missing fields as zeroumfDisjointPoolOpsV1()returns the older structure, user duplicates thatops->sizebytes, thenmemcpysizeto set new fields to NULLumfDisjointPoolOpsV2())sizedlsymsizeof(struct)instead ofops->sizeThe text was updated successfully, but these errors were encountered: