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

Reuse strings for memory usage #14289

Closed
sebastienros opened this issue Sep 7, 2023 · 11 comments
Closed

Reuse strings for memory usage #14289

sebastienros opened this issue Sep 7, 2023 · 11 comments

Comments

@sebastienros
Copy link
Member

In razor code compilation we pass string like this: https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore/OrchardCore.DisplayManagement/Theming/ThemingViewsFeatureProvider.cs#L28

The generated code for each view will then have these local strings. Could we pass a string that points to a static reference, like "OrchardCore.DisplayManagement.Theming.ThemingViewsFeatureProvider.MvcVersion" instead of @"mvc.1.0.view"

c.f. #14117 (comment)

@sebastienros
Copy link
Member Author

@jtkech

@jtkech
Copy link
Member

jtkech commented Sep 7, 2023

Okay, I will do a PR.

Would these strings still allocated if we just remove the @ prefix?

@jtkech
Copy link
Member

jtkech commented Sep 8, 2023

We can use this one

public class MvcViewDocumentClassifierPass : DocumentClassifierPassBase
{
    public static readonly string MvcViewDocumentKind = "mvc.1.0.view";

@sebastienros
Copy link
Member Author

It's not the @. It's that this string is injected in all generated views, so instead we need to inject a reference to a static string. Assuming this still works. Good to use an existing one if the compiled view has access to this property.

@jtkech
Copy link
Member

jtkech commented Sep 8, 2023

Okay, this is what I did and that works, see #14290

Hmm, I thought that when we use an hard coded "someString" it is allocated once in the code part and then passed as a reference, and that we allocate each time when using "someString1" + "someString2".

@ShaneCourtrille
Copy link
Contributor

@jtkech That is my understanding of how interning works which seems confirmed with a bit of Googling. The @ might be causing issues as we see a TON of memory usage from the string "mvc.1.0.view" but @rjpowers10 didn't see many instances.

@jtkech
Copy link
Member

jtkech commented Sep 11, 2023

@ShaneCourtrille

Are you sure they are different instances, maybe the inclusive memory is not that high.

In #14290 we no longer use @"mvc.1.0.view", but in ThemingViewsFeatureProvider we only add 2 razor compiled items for our hard coded ThemeViewStart and ThemeLayout, so anyway we can't do anything for all others ones related to razor files, they are pre-compiled and embedded in the assembly.

Unless you are using razor runtime compilation.

  • Are you using razor runtime compilation?

But in LiquidViewsFeatureProvider we add a compiled item for each liquid files.

  • Are you using a lot of Liquid files?

@ShaneCourtrille
Copy link
Contributor

Yup.. I've verified the strings by extracting the values from a memory dump. I'm not convinced they are actually caused by OC at this point but I can't confirm since they don't have a gcroot. They're all just sitting around in gen2 for some reason not being collected.

@jtkech
Copy link
Member

jtkech commented Sep 11, 2023

Okay, at least we no longer use @"mvc.1.0.view" but "mvc.1.0.view" in #14290, in fact we use MvcViewDocumentClassifierPass.MvcViewDocumentKind.

I assume you are not using razor runtime compilation.

Are you using a lot of Liquid files?

@ShaneCourtrille
Copy link
Contributor

@jtkech That's where things get REALLY weird.. afaik we don't use MVC at all. We have a completely separate/custom React UI that calls OC via API calls. I am going to pull in your change and run a perf test to see if it has any effect.

@jtkech
Copy link
Member

jtkech commented Sep 11, 2023

Okay, let me know

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 a pull request may close this issue.

3 participants