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

Optimize FieldAccessor #335

Merged
merged 4 commits into from
Jan 15, 2025
Merged

Optimize FieldAccessor #335

merged 4 commits into from
Jan 15, 2025

Conversation

SergeiPavlov
Copy link
Collaborator

@SergeiPavlov SergeiPavlov commented Jan 14, 2025

This class is critical for the performance.

Most of them are value-type accessors (DefaultFieldAccessor<>).
Using FieldIndex field instead of FieldInfo.MappingInfo.Offset expression will save one pointer dereference.

Copy link

@snaumenko-st snaumenko-st left a comment

Choose a reason for hiding this comment

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

It'd be great to have some benchmark for this change.

@@ -56,8 +56,8 @@ private static FieldAccessor CreateFieldAccessor(Type accessorType, FieldInfo fi
accessorType.CachedMakeGenericType(field.ValueType),
BindingFlags.CreateInstance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance,
null, Array.Empty<object>(), null);
accessor.Field = field;
accessor.SetFieldInfo(field);

Choose a reason for hiding this comment

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

Thinking out loud: maybe we can convert all these accessors to structs and get rid of virtual call dispatching as well? E.g. make them generic arguments of FieldAccessor
Could it be possible?

And maybe we can replace SetFieldInfo with single ctor argument?

Copy link
Collaborator Author

@SergeiPavlov SergeiPavlov Jan 15, 2025

Choose a reason for hiding this comment

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

This operation .SetFieldInfo() invoked only once, during Domain building
Little profit of optimizing it.

And I don't see easy way to use Accesor.GetValue()/SetValue() without virtual functions.
It would be to emulate vtbl by some other means

Choose a reason for hiding this comment

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

I mean not only this operation is worth optimizing. But others as well. These accessors seem to be good candidates for compiler optimizations with devirtualizing. There is not much of shared code and actually there is little sense to use inheritance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had an idea to inherit *Accessor from FieldInfo
because they are always together and mutually referenced
that will save yet one pointer dereference during access

@SergeiPavlov SergeiPavlov merged commit da22ea2 into master-servicetitan Jan 15, 2025
4 checks passed
@SergeiPavlov SergeiPavlov deleted the fieldAccessor branch January 15, 2025 10:48
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