Skip to content
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

FreeMembers with struct chains #264

Open
kainino0x opened this issue Jan 11, 2024 · 5 comments
Open

FreeMembers with struct chains #264

kainino0x opened this issue Jan 11, 2024 · 5 comments
Labels
!discuss Needs discussion (at meeting or online) needs docs Non-trivial API contract needs to be documented. Orthogonal to open vs closed; remove when doc'd. non-breaking Does not require a breaking change (that would block V1.0)

Comments

@kainino0x
Copy link
Collaborator

kainino0x commented Jan 11, 2024

Jan 11 meeting:

  • Should FreeMembers iterate down the struct chain for you, or should we have FreeMembers individually on each item in the chain?
  • discussion of how this works in a C++ API for example
    • If C++ explicitly represents the chain, then is it OK if dropping the root of the chain frees the members of the rest of the chain?
    • If C++ flattens all chained types into one struct, then it doesn’t matter except that the C++ bindings may have to make multiple FreeMembers calls in a row
  • Tentatively it should iterate, but we don’t have any cases of this yet so TBD
@kainino0x kainino0x added has resolution Issue is resolved, just needs to be done non-breaking Does not require a breaking change (that would block V1.0) labels Jan 11, 2024
@kainino0x kainino0x added the needs docs Non-trivial API contract needs to be documented. Orthogonal to open vs closed; remove when doc'd. label May 28, 2024
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Sep 16, 2024
@kainino0x
Copy link
Collaborator Author

A question on Matrix chat about how to deal with this in C++ bindings (RAII) raised some issues:

  • FreeMembers frees a bunch of arrays that are passed by pointer. Is it guaranteed to handle some/all of them being nullptr? I say "yes" because (1) length=0,ptr=null represents an empty array which could be a valid output of some function and (2) it shouldn't just work on those particular arrays being null, it should also work on "empty" (zeroed) structs.

  • If no, wrappers must avoid calling FreeMembers on zeroed structs. If yes, should wrappers still try to avoid it? There is a tradeoff between:

    • complexity/time in the bindings, checking all of the arrays for null (recursively through the struct chain? [1])
      • The performance cost should be optimizable with inlining, but if the binding function is complex enough then it is less likely to get inlined. And bindings code complexity is annoying in its own right.
    • unnecessary calls into the C API
      • If the C API is dynamically linked or called by function pointer, then that is an unoptimizable cost.

    [1]: Recursing through the struct chain isn't required for anything right now but will probably eventually happen. OTOH if we reverse the tentative decision above and have FreeMembers for each struct in the chain (instead of one recursive one for the head struct), then each struct can be individually RAII'd, avoiding recursion. Then perhaps we can rely on compilers to inline+dead-code-eliminate the FreeMembers on default-initialized/moved-from structs.

@kainino0x
Copy link
Collaborator Author

cc @Kangz

@kainino0x kainino0x removed the has resolution Issue is resolved, just needs to be done label Sep 17, 2024
@Kangz
Copy link
Collaborator

Kangz commented Sep 18, 2024

IMHO it should be "Yes" it handles it, as it makes it way simpler to use the API. I'm not concerned about slight overhead in FreeMembers or superfluous calls to it, because it should be mostly in the cold path.

@kainino0x
Copy link
Collaborator Author

FWIW, I didn't realize until recently, free and delete are guaranteed to no-op when passed a null pointer. So this is consistent with that.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 9, 2024

  • Tentatively it should iterate, but we don’t have any cases of this yet so TBD

Some reasons to not iterate - have a separate FreeMembers for each struct in the chain:

  • Managing the lifetimes is less touchy, you don't have to keep the whole chain alive to call FreeMembers on the base
    • Any bindings which have RAII but explicit chaining (like Dawn's C++ bindings currently) will find it easy to call FreeMembers for each separately and can avoid worrying about the freeing order
  • The thing mentioned above, RAII bindings would have to check the entire chain for non-nulls in order to check whether they need to call FreeMembers
  • Extensions can just add FreeMembers for their extension structs and not for the base structs they chain off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
!discuss Needs discussion (at meeting or online) needs docs Non-trivial API contract needs to be documented. Orthogonal to open vs closed; remove when doc'd. non-breaking Does not require a breaking change (that would block V1.0)
Projects
None yet
Development

No branches or pull requests

2 participants