Skip to content

Optimize ExpressionMaterializer.CreateEntity()#486

Merged
SergeiPavlov merged 2 commits into
master-servicetitanfrom
ExpressionMaterializer
May 13, 2026
Merged

Optimize ExpressionMaterializer.CreateEntity()#486
SergeiPavlov merged 2 commits into
master-servicetitanfrom
ExpressionMaterializer

Conversation

@SergeiPavlov
Copy link
Copy Markdown
Collaborator

Avoid double Dictionary lookup

@SergeiPavlov SergeiPavlov requested a review from snaumenko-st May 12, 2026 23:37
index = entityRegistry.Count;
entityRegistry.Add(expression, index);
}
ref var indexRef = ref CollectionsMarshal.GetValueRefOrAddDefault(entityRegistry, expression, out var exists);
Copy link
Copy Markdown

@snaumenko-st snaumenko-st May 12, 2026

Choose a reason for hiding this comment

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

IMHO too dangerous. And my previous benchmarks haven't indicated the significant performance boost. So probably it's not worth it

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Why can it be dangerous?
.NET implementation of Dictionary.TryAdd() uses it inside

And here is some benchmark showing the performance gain https://matthewcrews.com/blog/2022/03/performance-of-key-value-lookups-types/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's easy to use it wrong way. That's it

@SergeiPavlov SergeiPavlov merged commit a92d899 into master-servicetitan May 13, 2026
60 checks passed
@SergeiPavlov SergeiPavlov deleted the ExpressionMaterializer branch May 13, 2026 00:13
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.

2 participants