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

Add benchmarks #7963

Merged
merged 6 commits into from
Dec 6, 2022
Merged

Add benchmarks #7963

merged 6 commits into from
Dec 6, 2022

Conversation

chsienki
Copy link
Contributor

@chsienki chsienki commented Nov 27, 2022 •
edited
Loading

  • Add new benchmark project
  • Add sample app that can be benchmarked

You can pretty much ignore everything under SampleApp it's just a basic razor app with extra (simple) pages added under Pages/Generated to simulate a larger project.

@chsienki chsienki added area-infrastructure area-compiler Umbrella for all compiler issues labels Nov 27, 2022
@chsienki chsienki marked this pull request as ready for review November 29, 2022 21:36
@chsienki chsienki requested review from a team as code owners November 29, 2022 21:36
@chsienki
Copy link
Contributor Author

@dotnet/razor-compiler for review please.

333fred
333fred reviewed Nov 29, 2022
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass (commit 2)

jaredpar
jaredpar reviewed Nov 29, 2022
View reviewed changes
src/Compiler/perf/Microsoft.AspNetCore.Razor.Microbenchmarks.Generator/ProjectSetup.cs Outdated Show resolved Hide resolved
src/Compiler/perf/Microsoft.AspNetCore.Razor.Microbenchmarks.Generator/ProjectSetup.cs Outdated Show resolved Hide resolved
src/Compiler/perf/Microsoft.AspNetCore.Razor.Microbenchmarks.Generator/ProjectSetup.cs Outdated Show resolved Hide resolved
src/Compiler/perf/Microsoft.AspNetCore.Razor.Microbenchmarks.Generator/ProjectSetup.cs Outdated Show resolved Hide resolved
src/Compiler/perf/Microsoft.AspNetCore.Razor.Microbenchmarks.Generator/ProjectSetup.cs Outdated Show resolved Hide resolved
src/Compiler/perf/Microsoft.AspNetCore.Razor.Microbenchmarks.Generator/ProjectSetup.cs Outdated
{
private string _targetPath;

private AnalyzerConfigOptions _baseOptions;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private AnalyzerConfigOptions _baseOptions;
private readonly AnalyzerConfigOptions _baseOptions;

Copy link
Member

Choose a reason for hiding this comment

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

This wasn't addressed.

eng/Versions.props
@@ -184,5 +184,7 @@
<SystemTextJsonVersion>6.0.0</SystemTextJsonVersion>
<XunitAssertVersion>$(XunitVersion)</XunitAssertVersion>
<XunitExtensibilityExecutionVersion>$(XunitVersion)</XunitExtensibilityExecutionVersion>
<!-- Benchmarks -->
<Benchmarks_BaselineSourceGeneratorsVersion>7.0.0-preview.5.22528.1</Benchmarks_BaselineSourceGeneratorsVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Is this an existing pattern in arcade that we're feeding into or a new one we're craeting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a sort of pattern where we have things like Tooling_ it's not an official arcede-ism, but these are manually updated anyway, so arcade doesn't know about them

....Razor.Microbenchmarks.Generator/Microsoft.AspNetCore.Razor.Microbenchmarks.Generator.csproj Outdated
<PackageReference Include="Microsoft.Build.Locator" />
</ItemGroup>

<!-- Reference the local source generator when building in Release (or Debug) -->
Copy link
Member

Choose a reason for hiding this comment

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

Don't get the "or Debug" part here.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it means "when building in Release or Debug, but not in Release_Nuget"

@chsienki chsienki mentioned this pull request Dec 2, 2022
Open
@chsienki
Copy link
Contributor Author

chsienki commented Dec 2, 2022

Rebased on main, addressed feedback. @dotnet/razor-compiler for review please.

@333fred
Copy link
Member

333fred commented Dec 5, 2022

@chsienki can you please clarify which commits need to be reviewed? I would ask that you please do not rebase during code review, as it breaks the ability to diff review. This is critical, especially with PRs that have 2000 lines of diff.

@chsienki
Copy link
Contributor Author

chsienki commented Dec 5, 2022

@333fred it's just the last two commits which address PR feedback

333fred
333fred reviewed Dec 5, 2022
View reviewed changes
....Razor.Microbenchmarks.Generator/Microsoft.AspNetCore.Razor.Microbenchmarks.Generator.csproj Outdated
</ItemGroup>

</Project>

Copy link
Member

Choose a reason for hiding this comment

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

Spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@333fred What do you mean by spaces? The gap between the last item group and the closing project element follows the style of the rest of the repo.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that when I look at this in the commit diff, I see spaces:
image

333fred
333fred reviewed Dec 5, 2022
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass (commit 5)

@chsienki chsienki enabled auto-merge (squash) December 6, 2022 21:52
@chsienki chsienki merged commit 13ad9d5 into dotnet:main Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredpar jaredpar jaredpar left review comments

@jjonescz jjonescz jjonescz approved these changes

@333fred 333fred 333fred approved these changes

Assignees
No one assigned
Labels
area-compiler Umbrella for all compiler issues area-infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chsienki @333fred @jaredpar @jjonescz