-
Notifications
You must be signed in to change notification settings - Fork 226
[cpp] Overhaul resource ownership and record passing #1327
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the general direction of this patch, especially as it adds something which I have been planning to do for some time.
But I wonder whether the choice to move enums/records/typedefs into the same namespace as the exported functions/resources stems from not having a test which needs them unified/compatible between import and export.
Because I recall implementing the additional logic to satisfy that test case.
char* begin() { | ||
return (char*)data_; | ||
} | ||
char* end() { | ||
return (char*)data_ + length; | ||
} | ||
char const* begin() const { | ||
return (char const*)data_; | ||
} | ||
char const* end() const { | ||
return (char const*)data_ + length; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
@@ -113,7 +125,7 @@ template <class T> class vector { | |||
vector &operator=(vector const &) = delete; | |||
vector &operator=(vector &&b) { | |||
if (data_ && length>0) { | |||
free(const_cast<uint8_t *>(data_)); | |||
free(data_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the direction of this change I just recall that wit::vector<const T>
would trigger this, if I recall correctly. Probably the cause is removed by this new ownership design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see where that might be hit. I think it would be better to generate a const wit::vector<T>
instead of a wit::vector<const T>
@@ -1,30 +1,32 @@ | |||
#include <assert.h> | |||
#include <test_cpp.h> | |||
|
|||
std::tuple<uint8_t, uint16_t> exports::test::records::to_test::MultipleResults() { | |||
namespace test_exports = ::exports::test::records::to_test; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
} | ||
|
||
std::expected<float, ::test::results::test::E> exports::test::results::test::EnumError(float a) { | ||
auto result = ::test::results::test::EnumError(a); | ||
std::expected<float, test_exports::E> test_exports::EnumError(float a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the part I am less convinced about: That EnumError should be in exports.
@@ -28,8 +29,6 @@ pub const RESOURCE_EXPORT_BASE_CLASS_NAME: &str = "ResourceExportBase"; | |||
pub const RESOURCE_TABLE_NAME: &str = "ResourceTable"; | |||
pub const OWNED_CLASS_NAME: &str = "Owned"; | |||
pub const POINTER_SIZE_EXPRESSION: &str = "sizeof(void*)"; | |||
// these types are always defined in the non-exports namespace | |||
const NOT_IN_EXPORTED_NAMESPACE: bool = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find the test case this functionality aimed at, but there was a test case where the same interface was imported and exported by a world and enums, typedefs and structs should only be declared once to be compatible across imported and exported interface definitions.
Once we know how this works with the new ownership code, I am happy to get rid of this strange behavior.
I guess I simply need to create a test ;-)
/// Generated types used as parameters to imports will be "deeply | ||
/// borrowing", i.e. contain references rather than owned values | ||
/// for all fields that are not resources, which will be owning. | ||
FineBorrowing, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is similar enough to the Rust command line parameter to benefit from unification across these languages, but I can't perfectly map this proposal to Owning
vs Borrowing(copy_as_needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would coarse and fine also make sense for Rust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current Rust behaviour under Borrowing
is equivalent to the Coarse mode here. I'm not familiar enough with Rust's lifetime rules to say whether the Fine mode would make sense for Rust. Passing a record type containing a resource to an import under the Fine mode is like a partial move of the record, where only the resources are moved.
This spring Alex has rewritten the test infrastructure to no longer require a separate host side tester - making all of testing much simpler. |
This PR makes several core changes to the way that resources and records are managed in the C++ bindings. If there are any design changes you disagree with, we can work out a middle ground!
Ownership configuration
Introduces three resource ownership configurations for passing resources to imports:
std::string_view
), unless the record contains an owning handle, in which case the entire record will contain owning typesOwning is the default, as it is the easiest to use. Coarse borrowing is intended for uses where you want to avoid unnecessary allocation at the binding boundary. Fine borrowing is for when you really need to avoid those allocations, even if your record contains references, and you're willing to accept that the types become harder to reason about (because only the resource will be moved).
In the case of coarse borrowing and fine borrowing, a
<TypeName>Param
type will be unconditionally generated for each defined record type. This is different from the Rust bindings, which conditionally generates<TypeName>Result
and<TypeName>Param
, and takes a flag that enables or disables this behaviour. I figured that with three base types of ownership configuration, theduplicate-if-necessary
options would add unnecessary complication.See the following tests for examples of the three ownership types (it seems that these got into the last PR I made, whoops):
Reference types for record parameters to imports
Currently, imports with record parameters unconditionally take value types, which makes the API harder to use for some use cases, e.g. passing a record containing a
wit::vector
that you want to keep the contents of.This PR changes the parameter types of record parameters in a way that is dependent on the ownership configuration chosen:
T
contains an owning handleT
does not contain an owning handleT&&
T const&
T&&
TParam const&
TParam&&
TParam const&
This means that, if the type contains a resource, the user must
std::move
it, but otherwise they don't, and they don't incur a copy at the binding API boundary.To support this,
std::span
s in import arguments arestd::span<T>
if the type contains an owning handle andstd::span<T const>
otherwise.Namespaces of types
Currently there is no differentiation between imported types and exported types. This becomes a problem when resources are stored in records, because we have to store different types for imported and exported records.
This PR unconditionally puts exported types into the
exports
namespace, separating them from imported types. I believe this follows the behaviour of the Rust bindings.Borrow handles in records
Borrow handles are now correctly handled when they're stored in records, as previously they were treated as if they were in canonical form already.
There's still some work that needs done on resources inside
result
,option
,variant
, andtuple
, which will come in a future PR