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

Memoize InnerGetTypeName() #318

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

Conversation

SergeiPavlov
Copy link
Contributor

We see in tracing this is a hotspot

@alex-kulakov
Copy link
Contributor

Is ths a hotspot during Domain building or during domain live?

@SergeiPavlov
Copy link
Contributor Author

I don't remember.
By be just to make the code optimal for any case.

@alex-kulakov
Copy link
Contributor

alex-kulakov commented Aug 15, 2023

I think I know when you get hot paths for GetShortName(). Usage during Domain build corelates with number of model nodes (TypeInfo, FieldInfo, IndexInfo and TypeDef, FieldDef and so on). On HugeModeUpgrade.RegularModel I have 615 calls of GetShortName during domain build. Real problem is queries, especially TranslatorContext.GetApplyParameter(). During validation of populated data, tests execute queries and the validation queries make 2600 additional calls of GetShortName.

In various cases we can replace GetShortName with System.Type.Name, it is valid replacement for types which are guaranteed to be not Nested and not GenericType. Providers are among them. I have found more cases when we can make such replacement.

I see that System.RuntimeType has cache for names (according to the sources)

        public override String Name 
        {
            get 
            {
                return GetCachedName(TypeNameKind.Name);
            }
        }

Implicit usage of this cache will help us to not bloat our caches.

Maybe we can start with something like this (#333) and see how it goes. What do you think? I don't know whether you can check results of the PR I mentioned and tell me the outcome.

@SergeiPavlov
Copy link
Contributor Author

I don't mind these changes, but to test them with our app requires big efforts to create special branches and build.
So I'm going to wait until they will be merged into main branch

@alex-kulakov
Copy link
Contributor

Sure, as I thought, it is understandable.

@alex-kulakov
Copy link
Contributor

I merged my PR, I postpone this one for results of my PR, whether it did something or not.

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