Strip insignificant whitespace at compile time #23385

SteveSandersonMS · 2020-06-26T13:16:14Z

This is part of the perf optimization effort for Blazor.

From profiling, I've found that in intense rendering scenarios, we spend a lot of time dealing with whitespace. Even as much as half the total time. This is because (1) a lot of the nodes are just whitespace, and (2) whitespace is extra-expensive to insert because it gets treated as markup fragments (which trigger a separate HTML parsing call on each insertion).

As an example, consider this typical component code:

<ul>
    @foreach (var todoItem in TodoItems)
    {
        <li>
            @todoItem.Text is assigned to @todoItem.Assignee
        </li>
    }
</ul>

Looks harmless. But it generates whitespace nodes all over the place, due to spaces and linebreaks:

<ul>
HERE@foreach (var todoItem in TodoItems)
    {
HERE        <li>
HERE            @todoItem.Text is assigned to @todoItem.Assignee
HERE        </li>
HERE    }
HERE</ul>

It produces more whitespace nodes than anything else, and all of them are completely useless! They take up time in rendering, diffing, and in DOM-updating.

Proposed solution

Just like Angular and React do, we should by default trim insignificant whitespace nodes. Technically this will be a breaking change, because in theory all whitespace can be "significant" (e.g., if you set white-space: pre in CSS). However I think that:

It will be pretty rare for anyone to be depending on the particular patterns I propose to strip out
It will be very easy to turn off this behavior either on a per-component or per-project basis, if anyone wants
The perf gains are too big not to do this

In this PR, I've beefed up ComponentWhitespacePass so that it removes:

Whitespace nodes that are leading or trailing within an element
Whitespace nodes that are leading or trailing within a component child content block, since they play the same role as elements in terms of being containers
Whitespace nodes that immediately precede or follow a C# block (though not a C# expression). If you inspect the example case I posted above you'll see why that's needed.

Notably, I'm not proposing to remove whitespace from between sibling elements, whether or not they are block elements. That's because (1) I don't want to hard-code in knowledge about which element types are typically inline versus block, as it can be changed anyway via CSS and Razor isn't meant to be coupled to HTML, and (2) that's one of the behaviors that upsets/confuses React developers, and (3) it's just not very common compared with leading/trailing whitespace given typical shapes of HTML.

I've also added a new directive that you can use to get back the older whitespace-preserving behavior if you want:

@preservewhitespace true

<div>This component can render all kinds of useless whitespace, and that's how I want it</div>

Like other directives, you can put this into your _Imports.razor if you want the preference to apply to an entire subdirectory or even your entire project. And you can override on a nested basis (e.g., @preservewhitespace false to get the stripping back on a specific component).

Since this is technically a breaking change, I'm keen to include this in the preview 7 release to get the maximum opportunity for developer feedback.

DamianEdwards · 2020-06-26T17:23:57Z

Would be interesting to consider what it take to backport the moral equivalent of this to Razor (views/pages). Folks have been asking for the ability to have Razor auto-strip whitespace for a long time and I like the opt-in/out via a directive approach here.

pranavkm · 2020-06-26T17:18:33Z

src/Mvc/test/WebSites/BasicWebSite/RazorComponents/FetchData.razor

@@ -1,6 +1,6 @@
 @using BasicWebSite.Services
 @inject WeatherForecastService ForecastService
-
+@preservewhitespace true


Intentional?

Yes, it means I don't have to change the test case in a weird way (which wouldn't really be beneficial since the MVC test case isn't meant to be about how the component does its rendering internally).

pranavkm · 2020-06-26T17:20:20Z

...r/Microsoft.AspNetCore.Razor.Language/src/Components/ComponentPreserveWhitespaceDirective.cs

+    internal static class ComponentPreserveWhitespaceDirective
+    {
+        public static readonly DirectiveDescriptor Directive = DirectiveDescriptor.CreateDirective(
+            "preservewhitespace",


Yaay precendence: http://www.xmlplease.com/xml/xmlspace/#s3.

pranavkm · 2020-06-26T17:26:40Z

src/Razor/Microsoft.AspNetCore.Razor.Language/src/RazorProjectEngine.cs

@@ -217,6 +217,7 @@ private static void AddComponentFeatures(RazorProjectEngineBuilder builder)
            ComponentLayoutDirective.Register(builder);
            ComponentPageDirective.Register(builder);
            ComponentTypeParamDirective.Register(builder);
+            ComponentPreserveWhitespaceDirective.Register(builder);


We might have to introduce a new LangVersion and do this only for RazorLangVersion = 5.0 or newer. We don't want this directive to start working for 3.0 projects.

Yup need to introduce a new lang version for this. The biggest concern is without a lang version if you use a "new" IDE aka 16.8 VS without a new lang version it'll offer this directive in completions and wont complain when you use it in your files. However, the second you build this will cause build-time errors because it'll be using a different SDK.

OK, sounds good. Thanks for the explanation!

@pranavkm has volunteered to help with defining the new lang version, since apparently there are some subtleties has has context on related to controlling which Razor extension gets used inside VS.

Hopefully we'll have this in time to all go in at once in time for preview 7 (but if not, we could define the razorlangversion separately afterwards).

…ation)

…ion >= 5.0

ElderJames · 2020-06-29T14:21:50Z

This PR is very important, and our project is plagued by this problem.

https://ant-design-blazor.github.io/en-US/components/radio

SteveSandersonMS · 2020-06-29T16:59:18Z

Ping @NTaylorMullen @ajaybhargavb for re-review now. Hoping this can be merged today for inclusion in preview 7!

ajaybhargavb

Looks great! None of my questions/suggestions should be blocking for this release and can be done in a follow-up.

ajaybhargavb · 2020-06-29T19:18:37Z

...r/Microsoft.AspNetCore.Razor.Language/src/Components/ComponentPreserveWhitespaceDirective.cs

+            builder =>
+            {
+                builder.AddBooleanToken(ComponentResources.PreserveWhitespaceDirective_BooleanToken_Name, ComponentResources.PreserveWhitespaceDirective_BooleanToken_Description);
+                builder.Usage = DirectiveUsage.FileScopedMultipleOccurring;


Any reason for this to be FileScopedMultipleOccurring instead of FileScopedSinglyOccurring?

I thought the multiple occurring was needed for overriding, but if you’re saying that will still work with single occurring I’ll change it.

From my understand it is to allow multiple directives in the same file. Singly occurring should still allow overriding what was set in _Imports.razor. @NTaylorMullen can you confirm?

ajaybhargavb · 2020-06-29T19:33:02Z

src/Razor/Microsoft.AspNetCore.Razor.Language/src/Components/ComponentWhitespacePass.cs

+                    if (node.Children[childIndex] is CSharpCodeIntermediateNode)
+                    {
+                        childIndex -= RemoveContiguousWhitespace(node.Children, TraversalDirection.Backwards, childIndex - 1);
+                        RemoveContiguousWhitespace(node.Children, TraversalDirection.Forwards, childIndex + 1);


Trying to understand what is happening here. Why is this restarting the search from wherever it traversed to in the backwards direction? Why not just start it from childIndex + 1 without doing the childIndex -= in the previous line. Could definitely use a comment here to explain.

I’ll add a comment. We have to adjust the index here to account for having mutated the collection we’re iterating over.

Ah, makes sense

ajaybhargavb · 2020-06-29T19:36:46Z

src/Razor/Microsoft.AspNetCore.Razor.Language/src/DirectiveDescriptorBuilderExtensions.cs

+                    description: description));
+
+            return builder;
+        }


Would this benefit from having an optional boolean token? So people can just write @preserveWhitespace if they want the old behavior.

TBH this will be so rarely used I’d prefer to force the more explicit usage. But if you feel differently we can always change this in the future.

My concern is not specific to this directive but do you envision more boolean directives in the future that might benefit from this being optional? If so, I think it'll be a little confusing for the users if there are some boolean directive that requires explicit values and some that don't. I don't feel strongly but just something to consider.

Totally reasonable points. Perhaps at the time where we first feel it's valuable to add optional boolean support, we can decide whether or not to apply that feature here too, as that won't be a breaking change.

ajaybhargavb · 2020-06-29T19:48:21Z

src/Razor/Microsoft.AspNetCore.Razor.Language/src/Legacy/CSharpCodeParser.cs

@@ -1417,6 +1418,22 @@ private void ParseExtensibleDirective(in SyntaxListBuilder<RazorSyntaxNode> buil
                                    return;
                                }
                                break;
+
+                            case DirectiveTokenKind.Boolean:


What does the tooling experience currently look like?
Need to update DesignTimeDirectiveTargetExtension to provide the appropriate design time support so that it suggests true and false as options.

Ok, good point!

SteveSandersonMS · 2020-06-29T21:20:20Z

Thanks for the feedback @ajaybhargavb! Since you’ve pointed out the remaining bits don’t need to block merging, and to ensure this makes it into preview 7, I think we should merge this now then I’ll do a follow-up that can go into preview 8.

@mkArtakMSFT Can you merge this for preview 7? Thanks!

* Define @preservewhitespace directive attribute * Have ComponentWhitespacePass respect preservewhitespace option * Tests for overriding the preservewhitespace option (and fix implementation) * Begin adding test infrastucture for ComponentWhitespacePass * Remove leading/trailing whitespace from markup elements recursively * Update baselines * Add test showing we don't remove whitespace between sibling elements * Remove whitespace before/after C# code statements (not expressions) * Update baselines * Slight improvements to test * Remove leading/trailing whitespace inside component child content * Update baselines * Fix Razor tests * Fix MVC test * Better fix for MVC test * CR: Make ComponentPreserveWhitespaceDirective conditional on langversion >= 5.0 Commit migrated from dotnet/aspnetcore@eb7693157894

SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Jun 26, 2020

ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 26, 2020

SteveSandersonMS marked this pull request as ready for review June 26, 2020 16:43

SteveSandersonMS requested review from ajaybhargavb, NTaylorMullen and a team June 26, 2020 16:43

SteveSandersonMS mentioned this pull request Jun 26, 2020

Could we eliminate ComponentMarkupEncodingPass? #23384

Closed

pranavkm reviewed Jun 26, 2020

View reviewed changes

jaredpar mentioned this pull request Jun 26, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures dotnet/runtime#34472

Closed

SteveSandersonMS added 16 commits June 29, 2020 11:19

Define @preservewhitespace directive attribute

c554bb6

Have ComponentWhitespacePass respect preservewhitespace option

10b026d

Tests for overriding the preservewhitespace option (and fix implement…

61a7779

…ation)

Begin adding test infrastucture for ComponentWhitespacePass

f789981

Remove leading/trailing whitespace from markup elements recursively

71330fb

Update baselines

1571395

Add test showing we don't remove whitespace between sibling elements

b78c2ee

Remove whitespace before/after C# code statements (not expressions)

47a5f7b

Update baselines

19c0f01

Slight improvements to test

f3e8aad

Remove leading/trailing whitespace inside component child content

6429e46

Update baselines

6ca5485

Fix Razor tests

291d6c8

Fix MVC test

538402e

Better fix for MVC test

547e3d0

CR: Make ComponentPreserveWhitespaceDirective conditional on langvers…

adef3df

…ion >= 5.0

SteveSandersonMS requested review from BrennanConroy, dougbu, halter73 and jkotalik as code owners June 29, 2020 11:05

SteveSandersonMS requested a review from Tratcher as a code owner June 29, 2020 11:05

SteveSandersonMS changed the base branch from master to release/5.0-preview7 June 29, 2020 11:05

SteveSandersonMS requested review from a team and removed request for halter73, Tratcher, dougbu, BrennanConroy and jkotalik June 29, 2020 11:06

ajaybhargavb approved these changes Jun 29, 2020

View reviewed changes

mkArtakMSFT merged commit eb76931 into release/5.0-preview7 Jun 30, 2020

mkArtakMSFT deleted the stevesa/razor-optimize-whitespace branch June 30, 2020 14:49

SteveSandersonMS mentioned this pull request Jun 30, 2020

Follow-up on CR items from #23385 (strip insignificant whitespace) #23514

Closed

pranavkm mentioned this pull request Jun 30, 2020

Add support for LangVersion 5.0 dotnet/razor#2148

Merged

DamianEdwards mentioned this pull request Jul 1, 2020

Extend Blazor "insignificant whitespace trimming" into Razor Pages and MVC. #23570

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip insignificant whitespace at compile time #23385

Strip insignificant whitespace at compile time #23385

SteveSandersonMS commented Jun 26, 2020 •

edited

Loading

DamianEdwards commented Jun 26, 2020

pranavkm Jun 26, 2020

SteveSandersonMS Jun 26, 2020

pranavkm Jun 26, 2020

pranavkm Jun 26, 2020

NTaylorMullen Jun 26, 2020

SteveSandersonMS Jun 26, 2020

SteveSandersonMS Jun 29, 2020

ElderJames commented Jun 29, 2020

SteveSandersonMS commented Jun 29, 2020

ajaybhargavb left a comment

ajaybhargavb Jun 29, 2020

SteveSandersonMS Jun 29, 2020

ajaybhargavb Jun 29, 2020

ajaybhargavb Jun 29, 2020

SteveSandersonMS Jun 29, 2020

ajaybhargavb Jun 29, 2020

ajaybhargavb Jun 29, 2020

SteveSandersonMS Jun 29, 2020

ajaybhargavb Jun 29, 2020

SteveSandersonMS Jun 30, 2020

ajaybhargavb Jun 29, 2020

SteveSandersonMS Jun 29, 2020

SteveSandersonMS commented Jun 29, 2020

Strip insignificant whitespace at compile time #23385

Strip insignificant whitespace at compile time #23385

Conversation

SteveSandersonMS commented Jun 26, 2020 • edited Loading

Proposed solution

DamianEdwards commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElderJames commented Jun 29, 2020

SteveSandersonMS commented Jun 29, 2020

ajaybhargavb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Jun 29, 2020

SteveSandersonMS commented Jun 26, 2020 •

edited

Loading