Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Too much string.Concat in generated code with large blocks of HTML #614

Closed
rynowak opened this issue Nov 18, 2015 · 19 comments
Closed

Too much string.Concat in generated code with large blocks of HTML #614

rynowak opened this issue Nov 18, 2015 · 19 comments
Assignees
Milestone

Comments

@rynowak
Copy link
Member

rynowak commented Nov 18, 2015

Take a gander at the generated code for a page with a lot of 'literal text'

https://github.com/rynowak/mvc-sandbox/blob/master/workloads/RazorCodeGenerator/LargePage.cshtml

namespace Test
{
    using System;
    using System.Linq;
    using System.Collections.Generic;
    using Microsoft.AspNet.Mvc;
    using Microsoft.AspNet.Mvc.Rendering;
    using System.Threading.Tasks;

    public class LargePage : Microsoft.AspNet.Mvc.Razor.RazorPage<dynamic>
    {
        #line hidden
        public LargePage()
        {
        }
        #line hidden
        [Microsoft.AspNet.Mvc.Razor.Internal.RazorInjectAttribute]
        public Microsoft.AspNet.Mvc.IUrlHelper Url { get; private set; }
        [Microsoft.AspNet.Mvc.Razor.Internal.RazorInjectAttribute]
        public Microsoft.AspNet.Mvc.IViewComponentHelper Component { get; private set; }
        [Microsoft.AspNet.Mvc.Razor.Internal.RazorInjectAttribute]
        public Microsoft.AspNet.Mvc.Rendering.IJsonHelper Json { get; private set; }
        [Microsoft.AspNet.Mvc.Razor.Internal.RazorInjectAttribute]
        public Microsoft.AspNet.Mvc.Rendering.IHtmlHelper<dynamic> Html { get; private set; }

        #line hidden

        #pragma warning disable 1998
        public override async Task ExecuteAsync()
        {
            BeginContext(0, 160056, true);
            WriteLiteral("<p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
"p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
...

Yeah, that's a string.Concat that's going to make a 160056 character string.

We need to make Razor smarter.

@Eilon
Copy link
Member

Eilon commented Nov 19, 2015

I'm surprised that this is using string.Concat() at all. Why does the compiler not fold these into one large string literal? Is this about debug/release? An optimization that doesn't exist in Roslyn perhaps?

@Eilon
Copy link
Member

Eilon commented Nov 19, 2015

For example, this code:

            var x = "a" + "b";
            Console.WriteLine(x);

Compiled into this (via ILSpy):

    string value = "ab";
    Console.WriteLine(value);

And similar code is in the IL:

    IL_0008: ldstr "ab"

@Eilon
Copy link
Member

Eilon commented Nov 19, 2015

I tried with strings that add up to ~300KB and I got the same results each time: all the string literals were folded (feld? felled? felded? follied?) into one giant string.

The form of my strings was the same as in the example:

            var x =
                "p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
                "p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><p>Paragraph</p><" +
    ...

@rynowak
Copy link
Member Author

rynowak commented Nov 20, 2015

Per @NTaylorMullen - this semantic was copied from CodeDOM, and was intended to avoid an LOH allocation. It doesn't sound like that's doing what it's supposed to do. We might need to measure more to see what's better for GC perf here - allowing the LOH string vs breaking it up into separate writes.

Ok with pushing this item out until we're in a better position to test this.

@Eilon
Copy link
Member

Eilon commented Nov 20, 2015

My only point was there there's no string.Concat at all in the code shown. Just one ginormous string (which has to end up somewhere in random access RAM memory).

@rynowak
Copy link
Member Author

rynowak commented Nov 20, 2015

Yeah, you're totes bringing the pain. We should still look at this though.

@rynowak
Copy link
Member Author

rynowak commented Dec 10, 2015

@Eilon @NTaylorMullen - this causes an OOM sometimes during runtime compilation:

>   mscorlib.dll!string.Concat(string str0, string str1)    Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.FoldStringConcatenation(Microsoft.CodeAnalysis.CSharp.BinaryOperatorKind kind, Microsoft.CodeAnalysis.ConstantValue valueLeft, Microsoft.CodeAnalysis.ConstantValue valueRight, ref int compoundStringLength)    Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.FoldBinaryOperator(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode syntax, Microsoft.CodeAnalysis.CSharp.BinaryOperatorKind kind, Microsoft.CodeAnalysis.CSharp.BoundExpression left, Microsoft.CodeAnalysis.CSharp.BoundExpression right, Microsoft.CodeAnalysis.SpecialType resultType, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, ref int compoundStringLength) Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindSimpleBinaryOperator(Microsoft.CodeAnalysis.CSharp.Syntax.BinaryExpressionSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, Microsoft.CodeAnalysis.CSharp.BoundExpression left, Microsoft.CodeAnalysis.CSharp.BoundExpression right, ref int compoundStringLength)  Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindSimpleBinaryOperator(Microsoft.CodeAnalysis.CSharp.Syntax.BinaryExpressionSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics) Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionInternal(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, bool invoked, bool indexed) Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindArgumentAndName(Microsoft.CodeAnalysis.CSharp.AnalyzedArguments result, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, bool hadError, Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode argumentSyntax, Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax argumentExpression, Microsoft.CodeAnalysis.CSharp.Syntax.NameColonSyntax nameColonSyntax, Microsoft.CodeAnalysis.RefKind refKind, bool allowArglist)   Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindArgumentAndName(Microsoft.CodeAnalysis.CSharp.AnalyzedArguments result, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, bool hadError, Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentSyntax argumentSyntax, bool allowArglist, bool isDelegateCreation) Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindInvocationExpression(Microsoft.CodeAnalysis.CSharp.Syntax.InvocationExpressionSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics) Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionInternal(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, bool invoked, bool indexed) Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindExpression(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, bool invoked, bool indexed) Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionStatement(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode node, Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax syntax, bool allowsAnyExpression, Microsoft.CodeAnalysis.DiagnosticBag diagnostics)   Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindStatement(Microsoft.CodeAnalysis.CSharp.Syntax.StatementSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics)   Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindBlock(Microsoft.CodeAnalysis.CSharp.Syntax.BlockSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, Microsoft.CodeAnalysis.CSharp.Binder blockBinder) Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.MethodCompiler.BindMethodBody(Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol method, Microsoft.CodeAnalysis.CSharp.TypeCompilationState compilationState, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, out Microsoft.CodeAnalysis.CSharp.ImportChain importChain)  Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethod(Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol methodSymbol, int methodOrdinal, ref Microsoft.CodeAnalysis.CSharp.Binder.ProcessedFieldInitializers processedInitializers, Microsoft.CodeAnalysis.CSharp.SynthesizedSubmissionFields previousSubmissionFields, Microsoft.CodeAnalysis.CSharp.TypeCompilationState compilationState)    Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedType(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol containingType)   Unknown
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedTypeAsTask.AnonymousMethod__0()  Unknown
    Microsoft.CodeAnalysis.dll!Roslyn.Utilities.UICultureUtilities.WithCurrentUICulture.AnonymousMethod__0()    Unknown
    mscorlib.dll!System.Threading.Tasks.Task.InnerInvoke()  Unknown
    mscorlib.dll!System.Threading.Tasks.Task.Execute()  Unknown
    mscorlib.dll!System.Threading.Tasks.Task.ExecutionContextCallback(object obj)   Unknown
    mscorlib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx)   Unknown
    mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx)   Unknown
    mscorlib.dll!System.Threading.Tasks.Task.ExecuteWithThreadLocal(ref System.Threading.Tasks.Task currentTaskSlot)    Unknown
    mscorlib.dll!System.Threading.Tasks.Task.ExecuteEntry(bool bPreventDoubleExecution) Unknown
    mscorlib.dll!System.Threading.Tasks.Task.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem() Unknown
    mscorlib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch()    Unknown
    mscorlib.dll!System.Threading._ThreadPoolWaitCallback.PerformWaitCallback() Unknown

We need to split this up into smaller chunks

@Eilon
Copy link
Member

Eilon commented Dec 10, 2015

@rynowak should we also be filing a Roslyn bug? The memory quantities in play here are not really that much. A few hundred KB is a drop in the bucket even in a 32bit process.

@rynowak
Copy link
Member Author

rynowak commented Dec 10, 2015

I think we should file a Roslyn bug if we're still seeing this on dotnet cli. We already dump a ton of stuff into LOH, and it's fragmentation that's likely causing this.

Also, this is likely going on in addition to the other allocations Razor is doing.

Derp

I think I'll go file that bug now.

image

@Eilon
Copy link
Member

Eilon commented Dec 10, 2015

Nice memory stress test 😄

@rynowak
Copy link
Member Author

rynowak commented Dec 10, 2015

dotnet/roslyn#7398

@davidfowl
Copy link
Member

This is actually known and also happens when we implemented the roslyn codedom based compiler.

/cc @DamianEdwards

@rynowak
Copy link
Member Author

rynowak commented Dec 10, 2015

It's pretty obviously known since there's a bug. Are you trying to make another point? We need to fix this.

@davidfowl
Copy link
Member

That we brought it up with the roslyn team back then and I believe the answer was to compile out of process to avoid bringing down the server with an OOM. I'm not sure there was a real solution per se.

@Eilon
Copy link
Member

Eilon commented Dec 10, 2015

But the OOM would happen anyway, no? I mean, something somewhere is going to get an OOM and compilation will fail. Don't care which process is crashing, my app doesn't work.

But either way we're going to need to avoid this problem in Razor so it's not as big an issue for us.

@DamianEdwards
Copy link
Member

The difference can be that one view/page fails to compile vs. the entire app.

@Eilon Eilon added this to the 4.0.0-rc2 milestone Dec 30, 2015
@Eilon
Copy link
Member

Eilon commented Dec 30, 2015

Razor needs to break up long literals every X chars and call WriteLiteral for each one. This avoid the Roslyn bug, and might even have other benefits.

I think the only question is what "X" should be. 1KB? 10KB? 50KB?

@dougbu
Copy link
Member

dougbu commented Jan 3, 2016

Could solve this bug by moving a special case from CSharpCodeWriter to CSharpCodeVisitor.

See CSharpCodeWriter.WriteStringLiteral() which special-cases any string shorter than 256 or longer than 1500 characters (both exclusive) or which contains a \0 character. In that special case, WriteCStyleStringLiteral() breaks the generated string into bits of at most 81 characters. Yes, the bits are joined together using "\" +\n\"".

Most WriteStringLiteral() callers deal with attributes names and values or similar strings that are highly unlikely to need such a broad special case (maybe one just for \0). Just two call sites (in CSharpCodeVisitor) seem to merit length-related special handling and both are generating calls to RazorPage.WriteLiteral() or RazorPage.WriteLiteralTo(). Do the length handling there instead.

(Side note: CSharpCodeWriter chose X to be 80 though that seems to be for style in the generated code.)

NTaylorMullen added a commit that referenced this issue Jan 5, 2016
- Roslyn currently has an issue where too large of strings result in out of memory exceptions at compile time. To combat this I broke down literal strings into 1k pieces each resulting in their own `WriteLiteral`/`WriteLiteralTo` calls.
- Added tests to validate large string rendering.

#614
NTaylorMullen added a commit that referenced this issue Jan 7, 2016
- Roslyn currently has an issue where too large of strings result in out of memory exceptions at compile time. To combat this I broke down literal strings into 1024 length pieces each resulting in their own `WriteLiteral`/`WriteLiteralTo` calls. The 1024 number corresponds directly with MVCs response string buffer.
- Added tests to validate large string rendering.

#614
@NTaylorMullen
Copy link
Member

605dcee

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

No branches or pull requests

7 participants