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

using virtual fuction instead of reflection #11513

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SimaTian
Copy link
Member

@SimaTian SimaTian commented Feb 27, 2025

###Part of #11160
Building on top of Eric's IPC pr - his work allowed for other bottlenecks to reveal themselves.

Context

read_from_stream_reflection
read_from_stream_virtual
Currently, the ReadFromStream function uses

ArgsReaderDelegate readerMethod = (ArgsReaderDelegate)CreateDelegateRobust(typeof(ArgsReaderDelegate), _buildEvent, methodInfo);

for a large portion of it's packet deserialization. This is large enough to be seen in the performance profiler as shown above more than 3% of CPU an it's on a critical path.
When I checked, the function was already virtual so the change is minimal. Unfortunately I had to make an allowance to the task host since it wasn't compatible.

Changes Made

Exposed a convenience public endpoint for the CreateFromStream function, that calls the virtual method Create from stream.
Used this endpoint instead of delegate creation.

Testing

As long as nothing breaks, I consider that a win.
I did local profiling.

Copy link
Member

@JanProvaznik JanProvaznik left a comment

Choose a reason for hiding this comment

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

could you please measure impact in perfstar?

@SimaTian
Copy link
Member Author

SimaTian commented Mar 12, 2025

I've set up a perf star run. It seems that this has a measurable impact for evaluation performance:
0.79% overall improvement for an evaluation hot run
0.33% overall improvement for an evaluation cold run
0.08% overall negative impact for build - I'm unsure about the reason, but it is small enough for me to claim that it is variance at hand.

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