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.

Sign up for GitHub

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

Jump to bottom

Avoid doing unncecessary work when generating component declaration files #24445

Merged
1 commit merged into from
Jul 31, 2020
Merged

Avoid doing unncecessary work when generating component declaration files #24445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/Razor/Microsoft.AspNetCore.Razor.Tools/src/GenerateCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ private int ExecuteCore(
if (GenerateDeclaration.HasValue())
{
b.Features.Add(new SetSuppressPrimaryMethodBodyOptionFeature());
b.Features.Add(new SuppressChecksumOptionsFeature());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents the pragma checksum from being generated on declaration files. The checksum is used by debugger. Declaration files aren't used as part of debugging, they're only used for component discovery. Having the checksum is actively harmful since any changes to the input would result in the checksum being different.

}

if (RootNamespace.HasValue())
Expand All @@ -227,6 +228,7 @@ private int ExecuteCore(
});

var results = GenerateCode(engine, sourceItems);
var isGeneratingDeclaration = GenerateDeclaration.HasValue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason the GenerateDeclaration.HasValue is cached here instead of called in the if-statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does a linq Any() call. It seems unnecessary to perform n-times (once for each code file we're code-generating).


foreach (var result in results)
{
Expand Down Expand Up @@ -255,6 +257,18 @@ private int ExecuteCore(
{
// Only output the file if we generated it without errors.
var outputFilePath = result.InputItem.OutputPath;
var generatedCode = result.CSharpDocument.GeneratedCode;
if (isGeneratingDeclaration)
{
// When emiting declarations, only write if it the contents are different.
// This allows build incrementalism to kick in when the declaration remains unchanged between builds.
if (File.Exists(outputFilePath) &&
string.Equals(File.ReadAllText(outputFilePath), generatedCode, StringComparison.Ordinal))
{
continue;
}
}

File.WriteAllText(outputFilePath, result.CSharpDocument.GeneratedCode);
}
}
Expand Down
60 changes: 57 additions & 3 deletions src/Razor/Microsoft.NET.Sdk.Razor/integrationtests/BuildIncrementalismTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,61 @@ async Task VerifyError()
}
}

[Fact]
[InitializeTestProject("MvcWithComponents")]
public async Task BuildComponents_DoesNotRegenerateComponentDefinition_WhenDefinitionIsUnchanged()
{
// Act - 1
var updatedContent = "Some content";
var tagHelperOutputCache = Path.Combine(IntermediateOutputPath, "MvcWithComponents.TagHelpers.output.cache");

var generatedFile = Path.Combine(RazorIntermediateOutputPath, "Views", "Shared", "NavMenu.razor.g.cs");
var generatedDefinitionFile = Path.Combine(RazorComponentIntermediateOutputPath, "Views", "Shared", "NavMenu.razor.g.cs");

// Assert - 1
var result = await DotnetMSBuild("Build");

Assert.BuildPassed(result);
var outputFile = Path.Combine(OutputPath, "MvcWithComponents.dll");
Assert.FileExists(result, OutputPath, "MvcWithComponents.dll");
var outputAssemblyThumbprint = GetThumbPrint(outputFile);

Assert.FileExists(result, generatedDefinitionFile);
var generatedDefinitionThumbprint = GetThumbPrint(generatedDefinitionFile);
Assert.FileExists(result, generatedFile);
var generatedFileThumbprint = GetThumbPrint(generatedFile);

Assert.FileExists(result, tagHelperOutputCache);
Assert.FileContains(
result,
tagHelperOutputCache,
@"""Name"":""MvcWithComponents.Views.Shared.NavMenu""");

var definitionThumbprint = GetThumbPrint(tagHelperOutputCache);

// Act - 2
ReplaceContent(updatedContent, "Views", "Shared", "NavMenu.razor");
result = await DotnetMSBuild("Build");

// Assert - 2
Assert.FileExists(result, generatedDefinitionFile);
// Definition file remains unchanged.
Assert.Equal(generatedDefinitionThumbprint, GetThumbPrint(generatedDefinitionFile));
Assert.FileExists(result, generatedFile);
// Generated file should change and include the new content.
Assert.NotEqual(generatedFileThumbprint, GetThumbPrint(generatedFile));
Assert.FileContains(result, generatedFile, updatedContent);

// TagHelper cache should remain unchanged.
Assert.Equal(definitionThumbprint, GetThumbPrint(tagHelperOutputCache));
}

[Fact]
[InitializeTestProject("MvcWithComponents")]
public async Task BuildComponents_RegeneratesComponentDefinition_WhenFilesChange()
{
// Act - 1
var updatedContent = "@code { [Parameter] public string AParameter { get; set; } }";
var tagHelperOutputCache = Path.Combine(IntermediateOutputPath, "MvcWithComponents.TagHelpers.output.cache");

var generatedFile = Path.Combine(RazorIntermediateOutputPath, "Views", "Shared", "NavMenu.razor.g.cs");
Expand Down Expand Up @@ -204,7 +254,7 @@ public async Task BuildComponents_RegeneratesComponentDefinition_WhenFilesChange
var definitionThumbprint = GetThumbPrint(tagHelperOutputCache);

// Act - 2
ReplaceContent("Different things", "Views", "Shared", "NavMenu.razor");
ReplaceContent(updatedContent, "Views", "Shared", "NavMenu.razor");
result = await DotnetMSBuild("Build");

// Assert - 2
Expand All @@ -222,8 +272,12 @@ public async Task BuildComponents_RegeneratesComponentDefinition_WhenFilesChange
tagHelperOutputCache,
@"""Name"":""MvcWithComponents.Views.Shared.NavMenu""");

// TODO:
Assert.Equal(definitionThumbprint, GetThumbPrint(tagHelperOutputCache));
Assert.FileContains(
result,
tagHelperOutputCache,
"AParameter");

Assert.NotEqual(definitionThumbprint, GetThumbPrint(tagHelperOutputCache));
}

[Fact]
Expand Down
8 changes: 7 additions & 1 deletion ...icrosoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Component.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- Used for tracking inputs to component generation -->
<_RazorComponentInputHash></_RazorComponentInputHash>
<_RazorComponentInputCacheFile>$(IntermediateOutputPath)$(MSBuildProjectName).RazorComponent.input.cache</_RazorComponentInputCacheFile>
<_RazorComponentDeclarationOutputCacheFile>$(IntermediateOutputPath)$(MSBuildProjectName).RazorComponent.output.cache</_RazorComponentDeclarationOutputCacheFile>
</PropertyGroup>

<ItemGroup>
Expand Down Expand Up @@ -85,7 +86,7 @@ Copyright (c) .NET Foundation. All rights reserved.
Name="RazorGenerateComponentDeclaration"
DependsOnTargets="$(RazorGenerateComponentDeclarationDependsOn)"
Inputs="$(MSBuildAllProjects);@(RazorComponentWithTargetPath);$(_RazorComponentInputCacheFile)"
Outputs="@(_RazorComponentDeclaration)"
Outputs="$(_RazorComponentDeclarationOutputCacheFile)"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this, we relied on the declaration files being older than the inputs to determine if the targets need to be executed. This now uses an explicit marker file. This allows declaration files to not be updated, but for us to track if we at least tried regenerating the declaration file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do the contents of this declaration file look like? Does the RazorGenerateComponentDeclaration task need to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The declaration files are the outline of the components with out any body (they kinda look like ref assemblies). It lets us know what components exist and what parameters are on them. Prior to this change, if you were editing Index.razor to change some markup, here's what the sequence of events would look like:

RazorGenerateComponentDeclaration
    Inputs: Index.razor
    Output: obj\Debug\net5.0\RazorDeclaration\Index.razor.g.cs

MSBuild would evaluate this target because `Index.razor` is newer than `Index.razor.g.cs` (since it was just edited). At the end of this target, `Index.razor.g.cs` would have a new timestamp but the contents would be unchanged.

Next up is the compile and tag helper discovery targets that we use to find out what changed in the compnents:

RazorCompileComponentDeclaration
    Inputs: obj\Debug\net5.0\RazorDeclaration\Index.razor.g.cs
    Output: obj\Debug\net5.0\RazorDeclaration\Assembly.declaration.dll

Since `Index.razor.g.cs` is newer, MSBuild will evaluate this target and perform CSC. Next up, since the declaration assembly is newer, we'll perform a tag helper discovery on it.


RazorGenerateComponentDeclaration
    Inputs: Index.razor
    Output: SomeMarkerFile

MSBuild would evaluate this target because `Index.razor` is newer than `SomeMarkerFile`. At the end of this target, `Index.razor.g.cs` would have not a have newer timestamp. This allows the next two targets to be skipped. SomeMarkerFile helps us track if the target was evaluated since the inputs were last changed.

Condition="'@(RazorComponentWithTargetPath->Count())'!='0'">

<ItemGroup>
Expand Down Expand Up @@ -120,8 +121,13 @@ Copyright (c) .NET Foundation. All rights reserved.
TagHelperManifest="$(_RazorComponentDeclarationManifest)"
GenerateDeclaration="true" />

<Touch
Files="$(_RazorComponentDeclarationOutputCacheFile)"
AlwaysCreate="true" />

<ItemGroup>
<FileWrites Include="@(_RazorComponentDeclaration)" />
<FileWrites Include="$(_RazorComponentDeclarationOutputCacheFile)" />
</ItemGroup>
</Target>

Expand Down