Unregister custom classes in reverse registration order #1047

Kehom · 2023-02-16T14:48:40Z

This should fix #914 , hopefully without any side effects.

akien-mga · 2023-02-16T15:03:22Z

Maybe we can use a different data structure than std::unordered_map in the first place if we actually care about the order?

Kehom · 2023-02-16T15:16:11Z

Maybe we can use a different data structure than std::unordered_map in the first place if we actually care about the order?

I thought about that for a moment. Yet I have seen several queries being performed within the code. I'm unsure of the performance impact of changing the data structure on those operations.

DmitriySalnikov · 2023-03-26T03:31:14Z

Do you have plans to update this PR or merge it?

saki7 · 2023-04-08T04:51:32Z

Maybe we can use a different data structure than std::unordered_map in the first place if we actually care about the order?

I thought about that for a moment. Yet I have seen several queries being performed within the code. I'm unsure of the performance impact of changing the data structure on those operations.

The complexity of querying std::unordered_map is O(1) in average cases, but when we do the same operation for std::vector the complexity is O(log2 n) (when binary search is performed).

Regarding the concept of ClassDB, we can clearly assume that the querying operations occur more often than insertion, so I would recommend keep using std::unordered_map.

The only overhead introduced by this PR is static std::vector<StringName> class_register_order, but it only exists in ClassDB (not in every single wrapped class), so we can live with it.

Of course the overhead could be eliminated by choosing other implementation, but even so we will need to save the insertion order as an independent data, if we are to preserve the querying efficiency. Such implementation and this PR's implementation would look very much alike.

@akien-mga I can still reproduce the original issue on current master, and I need some fix to be deployed. Could we merge this PR?

dsnopek

I get a segfault when testing this PR - details below!

Other than that, I think the approach in this PR is good. Regarding @akien-mga's comment:

Maybe we can use a different data structure than std::unordered_map in the first place if we actually care about the order?

I didn't do any benchmarking, but a std::map (which retains order) is slower to query than an std::unordered_map, and we're mostly querying classes.

See: https://www.geeksforgeeks.org/map-vs-unordered_map-c/

By adding an extra std::vector, we're increasing memory usage, but not by very much: a StringName is 8 bytes, and this vector only contains the custom classes registered in the given extension. If the extension registers 10 classes, that's only 80 bytes (plus whatever accounting is in std::vector, like the size).

But, in exchange for the extra memory, we're maintaining query performance of the classes unordered map. Since it's only this one time when uninitializing an extension that we care about the order, I think this an OK trade-off.

dsnopek · 2023-04-10T20:02:59Z

src/core/class_db.cpp

@@ -341,13 +342,16 @@ void ClassDB::initialize(GDExtensionInitializationLevel p_level) {
 }

 void ClassDB::deinitialize(GDExtensionInitializationLevel p_level) {
-	for (const std::pair<StringName, ClassInfo> pair : classes) {
-		const ClassInfo &cl = pair.second;
+	for (size_t i = class_register_order.size() - 1; i >= 0; i--)


I get a segfault when testing with this PR - the cause is the for loop here.

On my system at least, size_t is unsigned. This loop will run so long as i >= 0, which means that after i is 0 and it decrements again, it'll wrap around to the max value (rather than stopping the loop), and try to access beyond the length of the vector. And if the segfault didn't kill it, it would loop forever. :-)

If I change the loop to look like this:

for (std::vector<StringName>::reverse_iterator i = class_register_order.rbegin(); i != class_register_order.rend(); ++i) { const StringName &name = *i;

... then there is no segfault, and the bug this is attempting to fix is fixed!

Added that change. Forgot to consider the case of no registered custom classes!

Thanks!

FYI, this segfaults even when there are register custom classes, because i will eventually decrement to 0 (but not stop because the condition is i >= 0), and then i-- will flip it over to the maximum value rather than -1, because size_t is unsigned (at least on my system) and can't represent -1. I'm assuming it works for you because size_t is signed on your system?

In any case, with this change your PR is working great for me!

As far as I know size_t is a typedef and it's always unsigned. To be honest, now that you mentioned it I have no idea why the loop worked!

dsnopek · 2023-04-11T14:20:49Z

This looks good to me!

However, per Godot's PR workflow, you'll need squash this into a single commit. See the docs here for how to do that with an interactive rebase:

https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase

DmitriySalnikov · 2023-06-06T14:44:37Z

This branch has conflicts that must be resolved

Will it be merged before the 4.1 release?

dsnopek · 2023-06-06T14:54:52Z

Will it be merged before the 4.1 release?

Maybe! It's been on our list to discuss at GDExtension meetings for a while, we just haven't gotten to it.

However, it'd definitely need to be rebased with the conflicts resolved in order for it to be merged.

DmitriySalnikov · 2023-06-11T22:35:49Z

I can do the rebase myself. Should I do this?
But I'm not sure how to specify a co-author.

dsnopek · 2023-06-14T19:01:50Z

@DmitriySalnikov:

I can do the rebase myself. Should I do this?

If the original author doesn't respond after awhile or isn't interested in rebasing, then, yes, you can rebase it in a new PR!

But I'm not sure how to specify a co-author.

If you pull the branch this PR is based on, and do git rebase, it should retain the original author on the commit. However, there is also this convention used on GitHub, where you put a line like this at the end of the commit message:

Co-authored-by: name <additional-dev-1@example.com>

You can re-write the commit message via git rebase or git commit --amend

Kehom · 2023-06-14T22:53:08Z

Hello. Im deeply sorry for the amont of time it took for me to answer. Im out of town without access to my working machine. I will be back next saturday. Yuri Sarudiansky ***@***.***

…

________________________________ From: David Snopek ***@***.***> Sent: Wednesday, June 14, 2023 4:02 PM To: godotengine/godot-cpp ***@***.***> Cc: Kehom ***@***.***>; Author ***@***.***> Subject: Re: [godotengine/godot-cpp] Unregister custom classes in reverse registration order (PR #1047) @DmitriySalnikov<https://github.com/DmitriySalnikov>: I can do the rebase myself. Should I do this? If the original author doesn't respond after awhile or isn't interested in rebasing, then, yes, you can rebase it in a new PR! But I'm not sure how to specify a co-author. If you pull the branch this PR is based on, and do git rebase, it should retain the original author on the commit. However, there is also this convention used on GitHub, where you put a line like this at the end of the commit message: Co-authored-by: name ***@***.***> You can re-write the commit message via git rebase or git commit --amend — Reply to this email directly, view it on GitHub<#1047 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEUTOPNUGFG76HM6NYXJFHLXLIDCTANCNFSM6AAAAAAU6IU7V4>. You are receiving this because you authored the thread.Message ID: ***@***.***>

dsnopek

Discussed at the GDExtension meeting: we think using the seperate vector<T> is fine, especially since there won't be too many classes (only the classes registered in this GDExtension). Approved pending the rebase!

dsnopek · 2023-06-20T13:25:03Z

Thanks! And congrats for your first merged Godot contribution 🎉

Kehom requested a review from a team as a code owner February 16, 2023 14:48

akien-mga added the bug This has been identified as a bug label Feb 16, 2023

dsnopek requested changes Apr 10, 2023

View reviewed changes

Kehom force-pushed the master branch from 59a2145 to 3808184 Compare April 11, 2023 14:55

dsnopek approved these changes Apr 11, 2023

View reviewed changes

Calinou added the topic:gdextension This relates to the new Godot 4 extension implementation label Jun 11, 2023

dsnopek approved these changes Jun 16, 2023

View reviewed changes

Unregister custom classes in reverse registration order

20be441

Kehom force-pushed the master branch from 3808184 to 20be441 Compare June 19, 2023 14:05

dsnopek merged commit 2377f7e into godotengine:master Jun 20, 2023

akien-mga added this to the 4.1 milestone Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unregister custom classes in reverse registration order #1047

Unregister custom classes in reverse registration order #1047

Kehom commented Feb 16, 2023

akien-mga commented Feb 16, 2023 •

edited

Loading

Kehom commented Feb 16, 2023

DmitriySalnikov commented Mar 26, 2023

saki7 commented Apr 8, 2023 •

edited

Loading

dsnopek left a comment •

edited

Loading

dsnopek Apr 10, 2023

Kehom Apr 11, 2023

dsnopek Apr 11, 2023 •

edited

Loading

Kehom Apr 11, 2023

dsnopek commented Apr 11, 2023

DmitriySalnikov commented Jun 6, 2023

dsnopek commented Jun 6, 2023

DmitriySalnikov commented Jun 11, 2023

dsnopek commented Jun 14, 2023

Kehom commented Jun 14, 2023 via email

dsnopek left a comment

dsnopek commented Jun 20, 2023

Unregister custom classes in reverse registration order #1047

Unregister custom classes in reverse registration order #1047

Conversation

Kehom commented Feb 16, 2023

akien-mga commented Feb 16, 2023 • edited Loading

Kehom commented Feb 16, 2023

DmitriySalnikov commented Mar 26, 2023

saki7 commented Apr 8, 2023 • edited Loading

dsnopek left a comment • edited Loading

Choose a reason for hiding this comment

dsnopek Apr 10, 2023

Choose a reason for hiding this comment

Kehom Apr 11, 2023

Choose a reason for hiding this comment

dsnopek Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Kehom Apr 11, 2023

Choose a reason for hiding this comment

dsnopek commented Apr 11, 2023

DmitriySalnikov commented Jun 6, 2023

dsnopek commented Jun 6, 2023

DmitriySalnikov commented Jun 11, 2023

dsnopek commented Jun 14, 2023

Kehom commented Jun 14, 2023 via email

dsnopek left a comment

Choose a reason for hiding this comment

dsnopek commented Jun 20, 2023

akien-mga commented Feb 16, 2023 •

edited

Loading

saki7 commented Apr 8, 2023 •

edited

Loading

dsnopek left a comment •

edited

Loading

dsnopek Apr 11, 2023 •

edited

Loading