Simplify GDType by storing all properties in a single unified map. Save ~12mb runtime RAM - #122751
Simplify GDType by storing all properties in a single unified map. Save ~12mb runtime RAM#122751Ivorforce wants to merge 1 commit into
GDType by storing all properties in a single unified map. Save ~12mb runtime RAM#122751Conversation
c0c7c22 to
ab7d862
Compare
c142860 to
21544be
Compare
| bool is_bitfield = false; | ||
| }; | ||
|
|
||
| struct Property { |
There was a problem hiding this comment.
Seems weird to call a method "property". Shouldn't the struct be called Member? Then SETGET could be changed to PROPERTY, because that's what it is.
#bikeshedding
There was a problem hiding this comment.
The naming was adopted from several earlier pieces of code referring to this concept as "property".
However, i agree it's a little confusing, especially since that means the "actual" properties ended up being called "setgets".
I'm open to changing it (e.g. members and properties) but I'd rather address that in a separate PR.
There was a problem hiding this comment.
I'll admit I doubted myself for a second when I saw methods being registered as a property in the previous PR. Had to search online to make sure I wasn't going crazy thinking that methods aren't properties.
I'd more than welcome a style PR to fix this naming "quirk".
21544be to
ca88ee0
Compare
…or rather, one per type, one aggregated).
ca88ee0 to
7bdffae
Compare
StarryWorm
left a comment
StarryWorm
left a comment
There was a problem hiding this comment.
Looks mostly good to me, some minor concerns/comments below.
| Locker::Lock lock(Locker::STATE_READ); | ||
|
|
||
| ClassInfo *type = classes.getptr(p_class); | ||
| for (const KeyValue<StringName, const MethodBind *> &kv : type->gdtype->get_method_map(p_no_inheritance)) { |
There was a problem hiding this comment.
This change (and similar ones below), which restores the behavior for disabled types from before #117599, should be listed in the additional information in the PR description imo, since it is a technically unnecessary functional change.
See comment thread here: #117599 (review)
I agree with the change, as it restores lost behavior; however, it may come with a slight performance cost because it needs to iterate up the inheritance tree. In the future, the type disabling system should be taken a good look at (and likely removed entirely; I haven't been able to find users yet).
There was a problem hiding this comment.
Oh right, I noted this bug while coding but ended up uploading the PR a few days later so I forgot to mention it.
Good thing you brought it up!
| *r_valid = false; | ||
| } | ||
| r_value = Variant(); | ||
| return true; |
There was a problem hiding this comment.
This changes the behavior for enums from returning false to returning true. Looking at callers of this method, it doesn't seem like it would pose an issue anywhere. It might actually give a slight performance boost to Object::get() in the enum case.
Someone more experienced with it would need to double-check that the resulting GDScript VM's OPCODE(OPCODE_GET_MEMBER) behavior change does not have any negative downstream impacts, as it would have returned an error and triggered OPCODE_BREAK in the past for enums in the DEBUG_ENABLED case (since this method would return false).
There was a problem hiding this comment.
Yes, this was an intentional decision: Object.Enum.Value resolves, so clearly Object.Enum is a claimed namespace. I'd consider the fact that it's not handled right now to be a bug, or at least an inconsistency in the program's logic.
But yea, good to highlight it!
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
4 participants
What problem(s) does this PR solve?
GDType: Consolidate integer constants in property map #122727ClassDBtoGDType. AccelerateObjectproperty access 1.6x #122596Reduces RAM usage of the editor by ~13mb.
This was done in follow-up to #122596, where a ~25mb increase was deemed acceptable for the performance improvement. This PR brings it back down by ~13mb (to around +12mb net plus).
Additional information
The reduction was achieved by consolidating all property information into a single map (or rather, one self + one aggregated) such that keyvalues are deduplicated.
As a side benefit (or if I'm being honest, perhaps the main benefit long term), the new code makes it structurally impossible to have colliding property names, which decreases the risk of bugs as this code evolves or is used.
It's also arguably more predictable, since all properties are accessed in the same way.
This also fixes a regression from earlier GDType PRs where disabled classes' methods are not hidden as expected.
Enums are now also part of an object's namespace:
Object.Enum.Valueresolves, so clearlyObjectclaims the memberEnum. Since enums are not types, there's nothing logical to return, but it IS handled in the same way that other non-existing namespaces are (e.g.Object.propertywhenpropertyhas no getter is also handled, though notably without erroring (which is also kind of odd)).Tests
I tested the RAM decrease with the test from #122596.