andlabs / libui

Simple and portable (but not inflexible) GUI library in C that uses the native GUI technologies of each platform it supports.
Other
10.69k stars 614 forks source link

Memory Management #390

Open gnarz opened 6 years ago

gnarz commented 6 years ago

Hi there, I made a binding of libui to a garbage collected language, and have hit a problem. Currently all container controls destroy their children when they are destroyed. However, in garbage collected languages, this violates the principle of least surprise. You do normally not expect an object to become invalid as long as you have a reference to it. What's more, you can't even tell that the object has become invalid, but the problem here is the object becoming invalid in the first place.

My current solution for this is to remove all children from container controls before I destroy them, but this does not work for uiGrid, which does not have a function to remov children, and menus in their current form are even more of a special case.

I would wish for all container widgets to have a method to delete all of their children (without destroying them, of course), or at least a method to delete individual widgets, paired with a way to actually find them. Like, the uiBox control has no way to determine the number of children, but the uiTab control has... plus a solution for menus, that addresses this problem.

mischnic commented 6 years ago

@parro-it How is this handled in libui-node?

parro-it commented 6 years ago

Hi @gnarz, we effectively already solved the same problem for libui-node. I'm on mobile Atm, will give you more details later...

simplexidev commented 6 years ago

I +1 the question about managing the GC, since I know I'm not doing it right in my bindings either.

parro-it commented 6 years ago

Well, like you probably already know, a libui control is represented in code by an instance of struct uiControl:

typedef struct uiControl uiControl;

struct uiControl {
    uint32_t Signature;
    uint32_t OSSignature;
    uint32_t TypeSignature;
    void (*Destroy)(uiControl *);
    uintptr_t (*Handle)(uiControl *);
    uiControl *(*Parent)(uiControl *);
    void (*SetParent)(uiControl *, uiControl *);
    int (*Toplevel)(uiControl *);
    int (*Visible)(uiControl *);
    void (*Show)(uiControl *);
    void (*Hide)(uiControl *);
    int (*Enabled)(uiControl *);
    void (*Enable)(uiControl *);
    void (*Disable)(uiControl *);
};

One of it's members is void (*Destroy)(uiControl *);. libui set this member on control creation to a specific function for every control. Normal controls get a standard function, while containers get a more specialized one, that take cares of destroying all their children.

libui then calls this function on the window instance on window close, or on single widgets instance if they are removed from their containers early.:

https://github.com/andlabs/libui/blob/9cf6c3faf560d0ab69a46a540780ae405323dde8/common/control.c#L5-L8

...

parro-it commented 6 years ago

In libui-node, we have an instance of struct control_handle that we use to wrap every libui control before sending it to the JS world.

During the wrap process, we patch the Destroy member of the uiControl instance to a custom function, and we take care of store the original Destroy in function in our struct control_handle wrapper:

https://github.com/parro-it/libui-napi/blob/d15b085eb1c0d847cbf685586fdfcd7bccf58ed7/src/control.c#L49-L57

struct control_handle *handle = calloc(1, sizeof(struct control_handle));
    handle->env = env;
    handle->events = calloc(1, sizeof(struct events_list));
    handle->children = create_children_list();
    handle->control = control;
    handle->ctrl_type_name = ctrl_type_name;
    handle->original_destroy = control->Destroy;
    control->Destroy = control_on_destroy;
    ctrl_map_insert(&controls_map, handle, control);

We also stored the libui control and the wrapper in a global map, that we'll use in control_on_destroy function to retrieve the wrapper given then libui control:

void control_on_destroy(uiControl *control) {
    LIBUI_NODE_DEBUG("A control is destroying.");

    struct control_handle *handle;
    ctrl_map_get(&controls_map, control, &handle);
    LIBUI_NODE_DEBUG_F("Control %s %p destroying.", handle->ctrl_type_name, handle);

    handle->original_destroy(control);
    ctrl_map_remove(&controls_map, control);
    clear_all_events(handle->events);

    clear_children(handle->env, handle->children);

    handle->is_destroyed = true;
}

Our function control_on_destroy takes care of cleaning the wrapper instance, and then call the original function to execute the original libui Destroy function implementation.

...

parro-it commented 6 years ago

To integrate the JavaScript memory management with a libui one, we implement in this two moments (control creation and control destroyed) the creation of a N-API reference.

Essentially N-API reference allow us to keep an instance of a JavaScript objects alive as we want to. So, our JavaScript object will be garbage collected only when: 1) they doesn't have any reference in current JS runtime, AND 2) they are not a uiControl wrapper currently displayed in a window.

Depending of which is the language you are writing the bindings for, I think you could use a similar approach to memory management.

I remember now that I never asked @andlabs if this usage of the Destroy method could fits with how he designed libui.

mischnic commented 6 years ago

I remember now that I never asked @andlabs if this usage of the Destroy method could fits with how he designed libui.

Well, uiControl is the only struct whose structure is "public", as opposed to the individual controls.

parro-it commented 6 years ago

Well, uiControl is the only struct whose structure is "public", as opposed to the individual controls.

Yes, I think it should be fine.

msink commented 6 years ago

We also stored the libui control and the wrapper in a global map

Maybe ask @andlabs to add void *data into uiControl struct? Retrieving from global map is O(N) operation, while with pointer in Control itself it will be O(1).

msink commented 6 years ago

Implemented this method for Kotlin - works perfectly! Don't like that intermediate map, but as temporary workaround - ok.

gnarz commented 6 years ago

Thanks for the detailed description, @parro-it. It seems that I failed to properly describe my problem. I have already solved the resource leaking, actually quite similar to your solution but without patching the Destroy field of the uiControl struct. Wasn't too hard, either, so I don't really see much need for changed to libui to aid with this (though a data pointer might be helpful for some language bindings). My problem is of a different, but related sort. Consider this:

You create a uiEntry, then a uiWindow. Add the uiEntry to the uiWindow while still holding a reference to it. Have the user enter something, then close the window, and here it gets destroyed (either by an intermittent run of the gc, or by explicitly calling the destroy method). Now, in what state is the uiEntry you created before? Obviously it has not been collected, because there is still a reference to it. But is it usable? Can you still retrieve the data the user entered from this uiEntry? I've read through the code of libui-node, and it does not seem to address this issue.

This is the problem I am trying to solve. For most containers, this is actually doable without too much hassle, as I wrote in the original post, by deleting the children of container controls before destroying them. So, for most containers, in my binding, the uiEntry is still fully operational, which is what I would expect from it, given that I still have a reference to it and did not explicitly destroy it.

However, it is currently not solvable for uiGrid containers, because there is no way to delete the children, and it is also not solvable for uiMenu controls, which are very different from any other controls in that they are not even explicitly bound to a window, but implicitly by first creating the menu and then the window, and also because the uiMenuItems don't actually exist independently of the uiMenu.

So, in order to address this problem, I would need some more assistance from @andlabs' libui. The things that come to mind are to either have a way to tell container controls to not destroy their children when they are destroyed, or to be able to delete all children from container controls. A third option would be to be able to find and delete individual widgets from container controls, as it is possible for box controls, for example. But I can see that this might become hard to manage for uiGrid controls, or even the planned uiTable controls, where the fine folks who work on it have ideas about allowing arbitrary controls in cells...

I hope I am clearer now about what problem I am trying to solve.

msink commented 6 years ago

For me now lifecycle is very clear - if Control was destroy() -ed - directly or indirectly - it will throw Exception on every operation, only if (!destroyed) still works for that Control. What is wrong here?

https://github.com/msink/kotlin-libui/blob/8be37c82000c780dc41c66d67b3c54fafe69198b/src/main/kotlin/widgets.kt#L45

/** Represents a GUI control (widget). It provides methods common to all Controls. */
abstract class Control(internal var _ptr: COpaquePointer?) {
    internal val ctl: CPointer<uiControl> get() = _ptr?.reinterpret() ?: throw Error("Control is destroyed")
    internal val ref = StableRef.create(this)
    internal val ctlDestroy = ctl.pointed.Destroy
    init {
        ctl.pointed.Destroy = staticCFunction(::onDestroy)
        controls[ctl] = this
    }
}
private var controls = mutableMapOf<CPointer<uiControl>, Control>()
private fun onDestroy(ctl: CPointer<uiControl>?) {
    with (controls[ctl!!] ?: throw Error("Control is destroyed")) {
        ctlDestroy?.invoke(ctl)
        controls.remove(ctl)
        ref.dispose()
        _ptr = null
    }
}

/** Returns `true` if Control was destroyed - in this case all other operations
 *  are invalid and will `throw Error("Control is destroyed")`. */
val Control.destroyed: Boolean get() = _ptr == null

/** Destroy and free all allocated resources. */
fun Control.destroy() = uiControlDestroy(ctl)

...
gnarz commented 6 years ago

@msink I agree that for you as the creator of the binding and someone who is rather familiar with the inner workings of libui, the lifecfycle is clear. But this is about the expectations of the potential users of my library (binding) and the principle of least surprise.

I try to keep the surprises and the amount the users need to know about the inner workings of libui to a minimum. And the thing is, it mostly works, except for uiGrid and uiMenu.

msink commented 6 years ago

Kotlin is garbage collected language, too. And objects of class Control will be alive until garbage collected, but very limited in functional if destroy()-ed. This paradigm is not new, did you ever see Java SWT framework for example?

mimecorg commented 6 years ago

This is not a usual pattern in garbage collected languages.

In C#, which is garbage collected, if you destroy a WinForms window, the child controls will be disposed as well and will throw an error when trying to use them. In fact, I would be very surprised if you could use a control in a window which no longer exists. So I think @msink's implementation is correct.

That said, I agree that uiGrid should have the remove method, without it it's hard for me to implement this container in Vuido, but I think that's a different issue (see also #328).

gnarz commented 6 years ago

@mimecorg surprise, you can :) At least on linux. And why shouldn't it, the controls do exist independently of a window. The alternative would be that a control should stop functioning as soon as it is removed from a container (not destroyed, just removed).

But again, this is not about being correct. I do think @msink 's implementation is correct, so is the libui-node implementation, and so is what I had before, which was basically the same. I want to take it a step further. What it needs for this to work is one more function, and what's more, one function that does exist for all other container types in one form or another.

msink commented 6 years ago

I'm not sure that there can be any guaranties on OS level that widget is fully functional after destroying container it was in.

andlabs commented 6 years ago

FWIW my current design is modelled after what GtkWidget does, except without refcounting. Better ideas are appreciated, especially since I'm never quite sure if there's a memory management issue in some other form.

gnarz commented 6 years ago

@andlabs I appreciate your design choices, no better idea. But how about that function to remove child controls from uiGrid‘s? It would add symmetry with the other containers.

parro-it commented 6 years ago

According to me, the current design of control destroying is perfectly fine, I would just add the user member as implemented by @msink in #392, to allow bindings authors to keep a reference to their wrappers without reverting to a map as we are doing now in libui-node.

If I correctly understand the counter-purpose, you plan to remove the destroying of controls when a windows is closed, just to destroying them when their references are garbage collected.

This could be acceptable and maybe even expected in a garbage-collected language, but what about non garbage-collected ones? Is the final user expected to manually destroy all the control instances? I think these could become unpractical to manage.

andlabs commented 6 years ago

Do your GC'd languages support pinning the underlying memory address? You'll have a hard time with a void *data pointer without a map if they don't. (Package ui has this issue.)

msink commented 6 years ago

Yes, Kotlin/Native do: StableRef.create(this) Golang doesn't?

parro-it commented 6 years ago

Do your GC'd languages support pinning the underlying memory address? You'll have a hard time with a void *data pointer without a map if they don't. (Package ui has this issue.)

Node.js has a reference data structure, the actual memory address of the JavaScript objects has to be retrieved every time they need to be accessed.

msink commented 6 years ago

you plan to remove the destroying of controls when a windows is closed, just to destroying them when their references are garbage collected.

Dont know about Lua, but in some GC-languages object destructor may be not called ever, so with this aporoach OS resources will be not freed. IMHO this is much worse than destroying widgets when window is closed.

Usually this problem solved on next level of abstraction - like MVP, MVVM and similar paradigms. Data should live in Model, not in Presenter.

gnarz commented 6 years ago

Hi,

If I correctly understand the counter-purpose, you plan to remove the destroying of controls when a windows

is closed, just to destroying them when their references are garbage collected.

Nope. The "counter-proposal" (it isn't one, it's an extension proposal, at best) is to have a means for the library user to remove all child controls from a container before destroying it. Or, alternatively, to optionally inhibit destruction of child controls when destroying a container. But as I seem to be the only one caring about this issue, I've dropped it and implemented this differently.

Gunnar