Skip to content
This repository has been archived by the owner on Aug 3, 2024. It is now read-only.

Statically declaring handler functions #238

Open
Timidger opened this issue Dec 27, 2018 · 4 comments
Open

Statically declaring handler functions #238

Timidger opened this issue Dec 27, 2018 · 4 comments

Comments

@Timidger
Copy link
Member

Currently the only way to declare new handler functions from ManagerHandlers is to dynamically allocate listeners along with state structs.

However, often it's not necessary since the same handler function is used and is part of a zero sized type (so there's no state). So it should be possible to avoid allocation in this case.

This can be achieved by adding a new option to the compositor::Builder that takes a <manager_type>::Builder for each function. Once it takes a static builder it will be a runtime error (at builder time) to construct a dynamic manager handler for that resource.

For actually allocating that static handler so that we can access the handler functions afterwards (which currently is achieved by using a repr(C) boxed struct) we could have the manager handler builder passed to us from the compositor builder internally build out to a repr(C) type that is somehow statically allocated (and implements the manager handler trait to call the functions it has as members).

Another option is to use a lazy_static! internally, but that incurs branching overhead on each access even if it is safe.

The last problem is how to solve this in the context of wayland_listener!, which currently assumes a handler is dynamically allocated along with its listener state. Ultimately the "correct" option might be to take all the $code that currently exists there and abstract it out to a function that takes an &ManagerTrait. That way it can reference a dynamically allocated or static object.

@Timidger Timidger added this to the 1.0 milestone Dec 27, 2018
@Timidger
Copy link
Member Author

This should work, as a means to statically allocate the listeners:

struct Bar;

struct Foo {
    first: Option<unsafe extern "C" fn(&mut Bar, *mut ())>
}

static mut foo: Foo = Foo { first: None };

This will compile down to a nullable pointer, while also being safe to check against at runtime. All we need is to write the boiler plate code that takes a builder that replaces the static instance with those functions and then re-write the wayland_listener! code so that it can reuse as much as that well oiled code as it can. That code is very annoying, so I'd rather not write it twice.

@Timidger
Copy link
Member Author

Timidger commented Dec 27, 2018

Adding the unsound tag until I have looked at the code in practice and ensured it is sound.

@Timidger
Copy link
Member Author

One issue with this approach is that it effectively adds two code paths to arrive at the same place.

Currently, we just use boxed ManagerHandler traits to arrive at resource objects which are responsible for cleaning themselves up properly (e.g. by Box::from_raw and then letting that drop after removing the listeners from the wl_list). Those ManagerHandlers are passed boxed into the compositor builder, prompting an initial allocation we want to also avoid.

We could change the resource creation to instead of returning a Box<ManagerHandler> to instead return a Builder, for example keyboard::Builder that will have internal options on how to build out the object. That way even from dynamically defined ManagerHandlers you can construct static resource objects that don't dynamically allocate.

However we still need a struct that has pointers to static functions passed to the builder for static manager handlers (which again, is mutually exclusive with the dynamic option because it doesn't make sense to listen for it twice, especially with our memory model) and that now means we need to define the signature for these callback functions twice: once in the struct for static allocation and again in the trait for dynamic allocation.

If there is a way to allow passing a T: ManagerHandler to the compositor builder this will solve this issue since it will all have the same interface (even with the useless &self that's fine).

@Timidger
Copy link
Member Author

Timidger commented Dec 27, 2018

Actually now that I work on this more I've come to the rather startling conclusion that the manager handlers don't actually need to be dynamically dispatched ever! There is only ever one variant and the data you want to store in them can just as easily be stored in the global compositor state instead.

This means you'll never have to pass Box into the compositor::Builder and instead you'll just pass the static functions with a sub-builder.

This can also change the return types of the managers to not be a box but rather a builder result that can either be None (for not dealing with it) or of the result of constructing a builder and either building up the functions and then calling build_static() or passing a boxed trait object in build_dynamic(Box::new(object)).

I think the easiest way to attack this whole problem is splitting this into two issues: one is that the managers should always be static, and the second being you should be able to choose for the resource handlers.

See #239.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

1 participant