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

C callback signature requires C++ bindings to allocate #201

Closed
kainino0x opened this issue Jul 28, 2023 · 7 comments
Closed

C callback signature requires C++ bindings to allocate #201

kainino0x opened this issue Jul 28, 2023 · 7 comments
Labels
async Asynchronous operations and callbacks has resolution Issue is resolved, just needs to be done

Comments

@kainino0x
Copy link
Collaborator

This was a long offshoot of #158. I should have moved it to a new thread a long time ago, but now that we have a proposed resolution I'm filing this so I can label it separately.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Jul 28, 2023

  • KN: Unlikely to find a solution to the move semantics problem.
  • KN: Proposing having two userdatas because it’ll still be helpful sometimes
  • CF: Would like to put everything in a struct
  • KN: Planning to add an enum too, could go in the struct. But if function is in the struct, we need many struct definitions
  • Agreed proposal: One struct type for each callback {mode, callback, userdata, userdata2}
    • Even though it’s a little annoying to write in C
    • The mode is anticipated to be added in WGPUFuture #199
// inline
f((WGPUBufferMapCallbackInfo){mode, callback, userdata, userdata2});

// out-of-line
WGPUBufferMapCallbackInfo info = {mode, callback, userdata, userdata2};
f(info);

// C++
f({mode, callback, userdata, userdata2});

We also discussed just having a single struct for {mode, userdata, userdata2} and putting the callback separately. But we thought this was slightly better and it doesn't matter a ton.

@kainino0x kainino0x added has resolution Issue is resolved, just needs to be done async Asynchronous operations and callbacks labels Jul 28, 2023
@kainino0x
Copy link
Collaborator Author

kainino0x commented Apr 25, 2024

April 25th meeting:

  • CF: I incorrectly thought we needed two. We just need one: a function pointer and a userdata pointer.
  • KN: We also determined we didn’t really need 2 in c++, but there is a pattern that takes advantage of it.
  • LK: I think we do need 2 unless you need to do an extra allocation to hold onto the userdata+function pointer, and pass it through. Can do without an allocation if we have two userdata pointers.
  • CF: Feels like you should be able to do the thing that rust does to avoid it. [rough explanation]
  • KN: Don’t understand how that works, but here is how we plan to use it:
  • (@cwfitzgerald will share how it would be done with Rust)

@eliemichel
Copy link
Collaborator

How do you handle the lifetime of lambdas in the C++ version? This is something I've never been happy with in my wrapper (allocating a unique_ptr and returning it as a handle that I leave the user responsible of keeping alive).

@kainino0x
Copy link
Collaborator Author

@eliemichel dawn hasn't done it yet, but in the linked gist, it's done manually with new/delete. (I seem to have forgotten to delete in wrapCallback_LT, but it's there in wrapCallback_L.)

@eliemichel
Copy link
Collaborator

Oh ok now I realize that wgpuCallCallbackV is a mock function that emulates an async call, and that with the userpointer+usersize API it is up to the backend to manage the lifetime of the user data as well as its copy.

FTR what my wrapper is doing is like this:

using ErrorCallback = std::function<void(ErrorType type, char const * message)>;

std::unique_ptr<ErrorCallback> Device::setUncapturedErrorCallback(ErrorCallback&& callback) {
	auto handle = std::make_unique<ErrorCallback>(callback);
	static auto cCallback = [](WGPUErrorType type, char const * message, void * userdata) -> void {
		ErrorCallback& callback = *reinterpret_cast<ErrorCallback*>(userdata);
		callback(static_cast<ErrorType>(type), message);
	};
	wgpuDeviceSetUncapturedErrorCallback(m_raw, cCallback, reinterpret_cast<void*>(handle.get()));
	return handle;
}

This only uses a single userdata API (which is all there is so far, right?) but then requires to make sure the user pointer returned by setUncapturedErrorCallback outlives the callback invocation.

@cwfitzgerald
Copy link
Collaborator

cwfitzgerald commented Apr 29, 2024

We would always do a single allocation:

use std::ffi::c_void;

extern "C" {
    fn wgpuMyOneShotCallback(func: extern "C" fn(*mut c_void), user_data: *mut c_void);
}

extern "C" fn my_oneshot_callback_wrapper<F>(raw: *mut c_void)
where
    F: FnOnce(),
{
    let raw_typed: *mut F = raw.cast();
    let alloc: Box<F> = unsafe { Box::from_raw(raw_typed) };
    alloc()
}

pub fn my_oneshot_callback<F>(func: F)
where
    F: FnOnce(),
{
    // If F captures nothing, Box::new doesn't allocate.
    let alloc: Box<F> = Box::new(func);
    let raw_typed: *mut F = Box::into_raw(alloc);
    let raw: *mut c_void = raw_typed.cast();
    unsafe {
        wgpuMyOneShotCallback(my_oneshot_callback_wrapper::<F>, raw);
    };
}

So we wouldn't hit this super magically zero allocation case, but that case really wouldn't work well in rust.

@kainino0x
Copy link
Collaborator Author

@lokokung is starting to add the second userdata in Dawn, and with it, C++ niceties for CallCallback2_T (using 2 userdatas) and CallCallback_LT (also with an optimization to avoid the allocation when the lambda is just a function pointer - the caller needed zero userdatas). Once stuff starts landing I can link to the generated code in codesearch to see what it actually looks like (won't have CallCallback_LT yet though).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async Asynchronous operations and callbacks has resolution Issue is resolved, just needs to be done
Projects
None yet
Development

No branches or pull requests

4 participants