Skip to content

Remove GetTypeInfo calls #469

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

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

Conversation

Bykiev
Copy link

@Bykiev Bykiev commented Apr 29, 2025

This PR removes GetTypeInfo calls, which improves performance

Bykiev added 3 commits April 29, 2025 21:54
This PR removes GetTypeInfo calls, which improves performance
@dadhi
Copy link
Owner

dadhi commented Apr 30, 2025

@Bykiev Thank you for removing the TypeInfos. Good riddance.
I will merge it later when I've done with #468.

@dadhi
Copy link
Owner

dadhi commented Apr 30, 2025

Ok, let's merge it now, when I am to start the work over for #468.

@dadhi
Copy link
Owner

dadhi commented Apr 30, 2025

@Bykiev Builds are failed, could you check.

@Bykiev
Copy link
Author

Bykiev commented Apr 30, 2025

Can you please help with running tests in FEC in VS? I see there is TestRunner project, but how to run them with VS? There is no tests in VS test explorer

@Bykiev Bykiev marked this pull request as draft April 30, 2025 17:02
@Bykiev
Copy link
Author

Bykiev commented Apr 30, 2025

Converted into draft for now...
It seems I was wrong type.GetProperties(BindingFlags.DeclaredOnly); is equal to these binding flags: BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly | BindingFlags.NonPublic and if all of them is specified the performance will be about the same. To gain some more performance specific flags should be specified.

@dadhi
Copy link
Owner

dadhi commented Apr 30, 2025

@Bykiev Yeah, I would look later.

To run the tests there are 2 TestRunner projects.
You can run the build.bat to run everything which mimics the CI.
Or run more quickish bt.bat and b.bat to run the tests only on the selected platforms.

@Bykiev Bykiev marked this pull request as ready for review April 30, 2025 17:48
@Bykiev
Copy link
Author

Bykiev commented Apr 30, 2025

@dadhi, should be ok now, can you please rerun tests on CI?

@dadhi
Copy link
Owner

dadhi commented May 2, 2025

@Bykiev
Let's keep this PR open, because don't like that there is no performance gain, but the code size is even bigger.
At least, to make it concise, the flags can be combined once and put into constants. I will check later after #468.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants