Skip to content

Conversation

pranavkm
Copy link
Contributor

As part of converting to source generators, one of the features we miss from MSBuild is the ability to no-op and cache
results operations if inputs are unchanged (target incrementalism). In this particular case, we're trying to
caching tag helper descriptors instead of discovering it every build.

One fairly simple solution is to cache descriptors for reference assemblies (that typically remain unchanged between compilations),
and the current assembly separately. The former can be cached and invalidated when metadatareferences change while the latter has a smaller set of types to search.

As part of converting to source generators, one of the features we miss from MSBuild is the ability to no-op and cache
results operations if inputs are unchanged (target incrementalism). In this particular case, we're trying to
caching tag helper descriptors instead of discovering it every build.

One fairly simple solution is to cache descriptors for reference assemblies (that typically remain unchanged between compilations),
and the current assembly separately. The former can be cached and invalidated when metadatareferences change while the latter has
a smaller set of types to search.
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 27, 2021
@pranavkm pranavkm requested a review from captainsafia January 28, 2021 18:53
@pranavkm pranavkm added this to the 6.0-preview2 milestone Jan 28, 2021
@pranavkm
Copy link
Contributor Author

This comes up as part of source generation work.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Looks legit. Left some stuff inline.

{
// Arrange
var testTagHelper = "TestAssembly.TestTagHelper";
var enumTagHelper = "Microsoft.CodeAnalysis.Razor.Workspaces.Test.EnumTagHelper";
Copy link
Member

Choose a reason for hiding this comment

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

How does this EnumTagHelper come in to play? I would expect that there is a project reference to Microsoft.CodeAnalysis.Razor.Workspaces.Test somewhere but I am missing where that happens...

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 test compilation references the test project which has these types. It kinda simulates having a project reference like Mvc or Components.

// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

#nullable enable
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nullable everything!

}

foreach (var reference in compilation.References)
if ((discoveryMode & TagHelperDiscoveryFilter.ReferenceAssemblies) == TagHelperDiscoveryFilter.ReferenceAssemblies)
Copy link
Member

Choose a reason for hiding this comment

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

Can we encapsulate these types of checks into two static methods: ShouldDiscoverFromReferenceAssemblies and ShouldDiscoverFromAppAssembly or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I were to create a method, it's tempting to also put the lookup inside the code which now results in an extra dictionary lookup per call. In the absence of it, it's a simple enum check, which seems like an overkill to have a method for.

@pranavkm pranavkm merged commit 0890654 into main Jan 29, 2021
@pranavkm pranavkm deleted the prkrishn/filter-tag-helpers branch January 29, 2021 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants