Skip to content
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

Only store one single unique Type nodes for each type #1304

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

oxisto
Copy link
Member

@oxisto oxisto commented Aug 28, 2023

Instead of gathering individual type node objects and then squashing them back together, we just make sure that registerType only returns the single unique type object. To reduce the number of Type nodes needed as input to registerType, the objectType function does a pre-check now, if a ObjectType with the given name already exists, so no new Type object is even created at all.

This also removes the "secondary type edge" interface, which was only needed to merge extra usages of types in nodes. Now all type nodes are always referring to the single instance.

@oxisto oxisto requested a review from konradweiss as a code owner August 28, 2023 19:57
@oxisto oxisto force-pushed the avoid-duplicate-type-nodes branch 2 times, most recently from 975788f to c05e277 Compare August 28, 2023 20:28
@oxisto oxisto changed the title Only store single unique Type nodes for each type Only store one single unique Type nodes for each type Aug 28, 2023
@oxisto oxisto force-pushed the avoid-duplicate-type-nodes branch from c05e277 to f9901ae Compare August 28, 2023 20:57
@oxisto oxisto force-pushed the avoid-duplicate-type-nodes branch 3 times, most recently from fec5fc0 to 7b82760 Compare August 29, 2023 08:30
Copy link
Collaborator

@konradweiss konradweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like some clarification on the KeySet and the use of isInstanceOf

@oxisto oxisto force-pushed the avoid-duplicate-type-nodes branch from 7b82760 to cc9d0fb Compare August 29, 2023 09:31
Instead of gathering individual type node objects and then squashing them back together, we just make sure that `registerType` only returns the single unique type object. However, the input into the function is still a `Type` object, which needs to be created temporarily (and then GC'd shortly after).

To reduce the number of `Type` nodes needed as input, the `objectType` function does a pre-check now, if a `ObjectType` with the given name already exists, so new `Type` object is even created. This only is possible for non-generics types though.
@oxisto oxisto force-pushed the avoid-duplicate-type-nodes branch from cc9d0fb to 3a07ea0 Compare August 29, 2023 10:16
@sonarcloud
Copy link

sonarcloud bot commented Aug 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

82.9% 82.9% Coverage
0.0% 0.0% Duplication

@oxisto oxisto merged commit 463ac60 into main Aug 29, 2023
@oxisto oxisto deleted the avoid-duplicate-type-nodes branch August 29, 2023 10:24
oxisto added a commit that referenced this pull request Aug 29, 2023
Instead of gathering individual type node objects and then squashing them back together, we just make sure that `registerType` only returns the single unique type object. To reduce the number of `Type` nodes needed as input to `registerType`, the `objectType` function does a pre-check now, if a `ObjectType` with the given name already exists, so no new `Type` object is even created at all.

This also removes the "secondary type edge" interface, which was only needed to merge extra usages of types in nodes. Now all type nodes are always referring to the single instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants