Skip to content

using virtual fuction instead of reflection#11513

Merged
SimaTian merged 7 commits intomainfrom
replace-create-delegate-with-virtual-method
Mar 13, 2025
Merged

using virtual fuction instead of reflection#11513
SimaTian merged 7 commits intomainfrom
replace-create-delegate-with-virtual-method

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.

@SimaTian SimaTian enabled auto-merge March 13, 2025 10:50
@SimaTian SimaTian merged commit a2cac59 into main Mar 13, 2025
9 checks passed
@SimaTian SimaTian deleted the replace-create-delegate-with-virtual-method branch March 13, 2025 11:47
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