Remove all manual reference counting

[mikke89]

Remove all manual reference counting

[viciious]

I'm all for it. Another solution would be to use pooled-allocated objects so basically they never get actually destroyed but then this isn't going to work for custom (user) DOM objects.

[mikke89]

I've started making this change in the remove_refcount branch. Element now uses unique_ptr and e.g. Context uses shared_ptr, among other changes. We are also seeing a nice little performance increase with this change.

With these changes, at least the user doesn't need to do RemoveReference everywhere, but I'm not sure if using smart pointers in the user-facing API is the prettiest thing either. Any feedback is appreciated.

[mikke89]

I feel more confident now with the latest changes, and merged it with the develop branch. See the updated readme for details and the resulting changes to the API.

[viciious]

I just realized that this is probably gonna be a showstopper for us. Reference counting is what our UI scripting language (Angelscript) uses for garbage collecting purposes so that worked in a natural way with libRocket but not so much with rmlUI. On top of that, rewriting all bindings to use the smart pointers is going to be such a chore, I probably won't bother.

[mikke89]

I understand your concern, and had the same ones with the Lua bindings as they also use its garbage collector. However, the changes required were really minimal, only affecting about 100 lines of code, most of which were trivial changes. There are very few changes to the bindings, as (non-owning) raw pointers are still used for the most part.

In my opinion, the pros really outweigh the cons here, and make the library a lot more friendly to new users.

[viciious]

How is that even supposed to work in case when pointers (handles) to script objects are passed around on stack or some other non-trivial cases? In Angelscript that would trigger the "AddReference" callback in the engine but what would it be supposed to do in case of a smart pointer?

From what I get, AS would require a wrapper class to hide away the fact that it's not working with real pointers.

[mikke89]

I'm not familar with AS, but generally you could still work with raw pointers. It's just that the ownership model has changed, so e.g. elements are owned by their parents. Your scripts cannot "co-own" an element for example. If you want extra safety, e.g. some warning instead of a crash when operating on a deleted element, you could make an indirection. For example that your handles are indices into a look-up array or a heap-allocated object.

[viciious]

I'm not familar with AS, but generally you could still work with raw pointers. It's just that the ownership model has changed, so e.g. elements are owned by their parents. Your scripts cannot "co-own" an element for example. If you want extra safety, e.g. some warning instead of a crash when operating on a deleted element, you could make an indirection. For example that your handles are indices into a look-up array or a heap-allocated object.

That means there's no guarantee that the raw pointer is going remain valid if scripts co-own the associated element - say, there's a scheduled timer function, which is supposed to run later. If in the meantime the parent document gets destroyed, that will almost certainly result in a crash.

[mikke89]

Yes, that's why I mentioned the indirection above. You could mark destroyed elements as null for example, and then it would be caught when retrieving from the look-up container.

[viciious]

Yeah, I had to use that exact approach in a couple of other cases but I'd rather not in this one - the DOM binding code is already a huge bloody mess. Adding another level of indirection on top of that can only make it worse...

[mikke89]

The poor man's solution is just to add all new elements into a std::unordered_set, and remove them on destruction, then see if the element is there.

There is another approach too when thinking about it, although it might be just as, if not more, complicated. That is, you can extend the lifetime of the element if you make your own ElementInstancers. When Release is called, just pass the ownership to the script.

[viciious]

The poor man's solution is just to add all new elements into a std::unordered_set, and remove them on destruction, then see if the element is there.

Yeah, I know...

There is another approach too when thinking about it, although it might be just as, if not more, complicated. That is, you can extend the lifetime of the element if you make your own ElementInstancers. When Release is called, just pass the ownership to the script.

Well, the script alone is probably not going to reliably determine whether the object isn't needed anymore. Some sort of reference counting is still going to be required in that case.

[mikke89]

Yeah, I know...

Alright, problem solved! :)

I mean, I know there is some amount of pain here for scripting purposes, but the advantages outweigh that in my opinion.

[viciious]

Oh well, maybe I can change the binding adapter to operate on smart pointers instead.. Just look at this mess: https://github.com/Qfusion/qfusion/blob/master/source/ui/as/asbind.h

[mikke89]

Right, I don't want to dive too deep into that :P
I'm happy if you give it a shot.

[viciious]

Right, I don't want to dive too deep into that :P

Neither do I - I absolutely loathe C++, especially the template-heavy variety :D

[mikke89]

How is this working out for you, are you okay with closing this now?

[viciious]

How is this working out for you, are you okay with closing this now?

Yup, feel free to close this issue whenever you feel like it :)

[mikke89]

I'm opening up this issue again to discuss it some more actually :P

One thing I don't like right now is that event dispatching is quite dangerous if you modify the DOM, in particular when removing elements in the path of the event propagation.

I think users would expect not to be able to erase the current element and then use it further in their own code. But not that it might potentially crash the event propagation inside the library, as it may currently do.

What do you think, should we make this safer?

I have been working on a possible solution to this. That is, enabling the creation of weak pointers to elements (and possibly other objects). These pointers do not take ownership, clearly, but instead they can observe when the object is destroyed, and tell the user of the weak pointer as such. Even if they are owned by something completely elsewhere (so we keep the owning unique pointers as is).

I think this could be useful for other purposes as well. Right now, I'm seeing about a 2-3% performance loss in the benchmark with my initial hacked together solution, I think we can get this back up somewhat. But it seems to be working well.

Thoughts?

[mikke89]

I'm closing this as it is now possible to obtain a a weak smart pointer (ObserverPtr) to Elements.