Skip to content

Change candidate resolution logic to allow assemblies depending on shared framework. #4440

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

Closed
wants to merge 1 commit into from

Conversation

NTaylorMullen
Copy link

  • This is required due to how AspNet Core is included via a framework reference now. In order to properly evaluate transitive projects that target AspNet Core 3.0 we'd need some level of information that they targeted a framework reference. This happens to be a missing feature in the sdk/nuget. Also, given that all of ASP.NET is now included via a framework reference the act of finding candidate assemblies to reduce the amount of assembly loads doesn't matter as much since we went from 100s of potential assemblies to only what the application currently targets.
  • Fixed a test issue where calling into BasicApi tests resulted in the attempted loading of EFCore.Design. Problem is EFCore.Design wasn't a dependency of our functional test project because of PrivateAssets="true" and therefore, when we tried to find candidate assemblies we read in the BasicApiTest.deps.json but operated on the functional tests.deps.json. I could have added the EFCore.Design package to the functional test project to fix this but given that the package wasn't being used choose to remove it instead.
  • Removed Mvc.Analyzers from AspNetCore.Mvc because it's no longer brought in via the shared framework. This was needed because with the new approach with always loading all dll's in an applications graph we'd attempt to load Mvc.Analyzers at which point we'd explode saying we couldn't load Roslyn (it's not available at runtime anymore).
  • Updated tests to reflect new expectations in regards to application part type loading.

Addresses #4332

…ared framework.

- This is required due to how AspNet Core is included via a framework reference now. In order to properly evaluate transitive projects that target AspNet Core 3.0 we'd need some level of information that they targeted a framework reference. This happens to be a missing feature in the sdk/nuget. Also, given that all of ASP.NET is now included via a framework reference the act of finding candidate assemblies to reduce the amount of assembly loads doesn't matter as much since we went from 100s of potential assemblies to only what the application currently targets.
- Fixed a test issue where calling into BasicApi tests resulted in the attempted loading of EFCore.Design. Problem is EFCore.Design wasn't a dependency of our functional test project because of PrivateAssets="true" and therefore, when we tried to find candidate assemblies we read in the BasicApiTest.deps.json but operated on the functional tests.deps.json. I could have added the EFCore.Design package to the functional test project to fix this but given that the package wasn't being used choose to remove it instead.
- Removed Mvc.Analyzers from AspNetCore.Mvc because it's no longer brought in via the shared framework. This was needed because with the new approach with always loading all dll's in an applications graph we'd attempt to load Mvc.Analyzers at which point we'd explode saying we couldn't load Roslyn (it's not available at runtime anymore).
- Updated tests to reflect new expectations in regards to application part type loading.

Addresses #4332
@@ -22,7 +22,6 @@
<ItemGroup Condition="'$(BenchmarksTargetFramework)' == ''">
<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="$(MicrosoftAspNetCoreAuthenticationJwtBearerPackageVersion)" />
<PackageReference Include="Microsoft.AspNetCore.Server.Kestrel" Version="$(MicrosoftAspNetCoreServerKestrelPackageVersion)" />
<PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="$(BenchmarksOnlyMicrosoftEntityFrameworkCoreDesignPackageVersion)" PrivateAssets="All" />
Copy link
Author

Choose a reason for hiding this comment

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

Fixed a test issue where calling into BasicApi tests resulted in the attempted loading of EFCore.Design. Problem is EFCore.Design wasn't a dependency of our functional test project because of PrivateAssets="true" and therefore, when we tried to find candidate assemblies we read in the BasicApiTest.deps.json but operated on the functional tests.deps.json. I could have added the EFCore.Design package to the functional test project to fix this but given that the package wasn't being used choose to remove it instead.

candidateEntry.Classification = classification;

return classification;
throw new InvalidOperationException(Resources.FormatCandidateResolver_DifferentCasedReference(library.Name));
Copy link
Author

Choose a reason for hiding this comment

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

I assume this can still happen if you have multiple framework references in a single project so let this be.

@@ -74,34 +74,6 @@ public void ResolveAssemblies_ReturnsRelatedAssembliesOrderedByName()
Assert.Equal(new[] { ThisAssembly, assembly2, assembly1, assembly3 }, result);
Copy link
Author

Choose a reason for hiding this comment

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

Removed several tests from this class that were either 1. not valuable or 2. no longer valid.

@@ -9,7 +9,6 @@
</PropertyGroup>

<ItemGroup>
<ProjectReference Include="..\Microsoft.AspNetCore.Mvc.Analyzers\Microsoft.AspNetCore.Mvc.Analyzers.csproj" PrivateAssets="None" />
Copy link
Author

Choose a reason for hiding this comment

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

Analyzers don't make their way into the shared framework anymore anyhow. Having this caused assembly load issues because the functional tests would attempt to load the analyzers project which in turn explode at runtime because there's no Roslyn. This is a more significant issue that we may want to document. Up for suggestions here.

@@ -472,23 +472,6 @@ public async Task TestingInfrastructure_InvokesCreateDefaultBuilder()
Assert.Equal("true", response);
}

[Fact]
public async Task ApplicationAssemblyPartIsListedAsFirstAssembly()
Copy link
Author

Choose a reason for hiding this comment

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

Boat loads of assemblies now and felt like the test wasn't very valuable.

@NTaylorMullen
Copy link
Author

Still need to benchmark the impact this has on startup performance with @sebastienros

@NTaylorMullen
Copy link
Author

When benchmarking this we ran into an issue where the benchmark app was using a super old school (project.json) invalidly packaged netstandard1.5 package and then barfed (Sigil). This really makes me think we need to try-catch around our resolution logic. @rynowak I know you had some super serious concerns about doing that though. Could you share those?

In the meanwhile, going to work on running a benchmark on one of the test apps that doesn't use Sigil.

@NTaylorMullen
Copy link
Author

So the results make me want to cry 😢

Description RPS CPU (%) Memory (MB) Avg. Latency (ms) Startup (ms) First Request (ms) Latency (ms)
baseline 14,207 86 365 20.8 849 172.9 1.3
After Change 14,774 88 441 19 1056 (+24%) 608.7 (+352%) 1.4
--server http://asp-perf-win:5001 --client http://asp-perf-load:5002 `
>> --database MySql --jobs https://github.com/raw/aspnet/AspNetCore/master/src/Mvc/benchmarkapps/BasicViews/benchmarks.json -n BasicViews.GetTagHelpers --no-warmup --duration 1 --self-contained -i 5 -x 1 --outputFile "C:\Users\nimullen\Documents\GitHub\AspNetCore\src\Mvc\src\Microsoft.AspNetCore.Mvc.Core\bin\Release\netcoreapp3.0\Microsoft.AspNetCore.Mvc.Core.dll"

@NTaylorMullen
Copy link
Author

Abandoning this. The perf results are too scary. Working with partner teams to get the right information into the deps file.

@NTaylorMullen NTaylorMullen deleted the nimullen/4332 branch December 7, 2018 01:06
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

👍

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 this pull request may close these issues.

3 participants