-
Notifications
You must be signed in to change notification settings - Fork 95
Identify DotvvmProperty with systematic 32-bit IDs #1867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a2f0889
to
4745066
Compare
All DotvvmProperties are now assigned 32-bit ids, which can be used for more efficient lookups and identifications. The ID is formatted to allow optimizing certain common operations and make the assignment consistent even when we initialize controls on multiple threads. The ID format is (bit 0 is (id >> 31)&1, bit 31 is id&1) * bit 0 - =1 - is property group * bits 16-30: Identifies the property declaring type. * bits 0-15: Identifies the property in the declaring type. - =0 - any other DotvvmProperty * bits 16-30: Identifies the property group * bits 0-15: Identifies a string - the property group member All IDs are assigned sequentially, with reserved blocks at the start for the most important types which we might want to adress directly in a compile-time constant. IDs put a limit on the number of properties in a type (64k), the number of property groups (32k), and the number of property group members. All property groups share the same name dictionary, which allows for significant memory savings, but it might be limiting in obscure cases. As property groups share the name-ID mapping, we do not to keep the GroupedDotvvmProperty instances in memory after the compilation is done. VirtualPropertyGroupDictionary will map strings directly to the IDs and back. Shorter unmanaged IDs allows for efficient lookups in unorganized arrays using SIMD. 8 property IDs fit into a single vector register. Since, controls with more than 8 properties are not common, we can eliminate hashing with this "brute force". We should evaluate whether it makes sense to keep the custom small table--optimized hashtable. This patch keeps that in place. The standard Dictionary`2 is also faster when indexed with integer compared to a reference type. Number of other places in the framework were adjusted to adress properties directly using the IDs.
…rtual GetValue This is now frequently needed when working only with the ID instead of working with DotvvmProperty. Without this information, we pay not only for virtual dispatch, but also the (more expensive) cost of looking up the DotvvmProperty instance. Since most properties use plain DotvvmProperty and don't need the polymorphism, we now avoid this cost by using the last bit of the property ID to indicate whether the property can use direct access.
ddd5fa4
to
fcf2744
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a systematic 32‑bit ID scheme for all DotvvmProperties along with associated changes to property registration, lookup and code‐generation logic. Key changes include updating DotvvmProperty and DotvvmPropertyGroup to use the new DotvvmPropertyId structure, modifying helper methods and code generation utilities accordingly, and updating capability property registration.
Reviewed Changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/Framework/Framework/Compilation/ControlTree/DotvvmPropertyGroup.cs | Updated property group registration to use property IDs and improved thread‐safety. |
src/Framework/Framework/Binding/DotvvmPropertyId.cs | Introduced 32‑bit DotvvmPropertyId structure with bitwise operations to distinguish property groups. |
src/Framework/Framework/Binding/*.cs | Updated various binding and property registration files to use the new ID scheme and adjusted related helper methods. |
Files not reviewed (1)
- src/Directory.Build.props: Language not supported
Comments suppressed due to low confidence (3)
src/Framework/Framework/Binding/DotvvmPropertyId.cs:39
- [nitpick] Using ‘(int)Id < 0’ to determine if the ID represents a property group may be non‐intuitive. Consider using an explicit bitmask check (e.g. checking the most significant bit) for improved clarity.
public bool IsPropertyGroup { get => (int)Id < 0; }
src/Framework/Framework/Binding/DotvvmPropertyId.cs:101
- [nitpick] The parameter name 'id' in IsInPropertyGroup is ambiguous. Renaming it to 'groupId' could better communicate that it represents a property group identifier.
public bool IsInPropertyGroup(ushort id) => (this.Id >> 16) == ((uint)id | 0x80_00u);
src/Framework/Framework/Binding/DotvvmPropertyIdAssignment.GroupMembers.cs:11
- [nitpick] The constant 'id' is in lowercase, which may be confusing given coding standards that favor PascalCase for constants. Consider renaming it to 'Id' to improve clarity and consistency.
public const ushort id = 1;
All DotvvmProperties now get assigned 32-bit IDs, which can be used for more efficient lookup and identification. The has a ID 16b + 16b structure to allow optimizing certain common operations and make the assignment consistent even when we initialize controls on multiple threads.
The ID format is (bit 0 is (id >> 31)&1, bit 31 is id&1)
All IDs are assigned sequentially, with reserved blocks at the start for the most important types which we might want to address directly in a compile-time constant.
IDs put a limit on the number of properties in a type (64k), the number of property groups (32k), and the number of property group members. All property groups share the same name dictionary, which allows for significant memory savings, but it might be limiting in obscure cases.
As property groups share the name-ID mapping, we do not to keep the GroupedDotvvmProperty instances in memory after the compilation is done. VirtualPropertyGroupDictionary will map strings directly to the IDs and back.
Shorter unmanaged IDs allows for efficient lookup in unorganized arrays using SIMD — 8 property IDs fit into a single vector register. Since controls with more than 8 properties are not common, we can actually be faster with brute force.
We should evaluate whether it makes sense to keep the custom hashtable optimized for small table. Currently, the patch still keeps this code in place. The standard Dictionary`2 is also faster when indexed with integer compared to a reference type, so the difference will be even less.
Number of other places in the framework were adjusted to address properties directly using the IDs.