Bring over a few changes from the TechEmpower repo #266

dougbu · 2017-05-22T23:50:33Z

incorporate aspnet/FrameworkBenchmarks@4830e2fc, "Only Prepare DbCommand once"
enable more scenarios e.g. "--scenarios plaintext" also enables "mvc/plaintext" scenario
optimize Content-Length setting in plaintext scenario
remove unused Microsoft.EntityFrameworkCore.Design dependency and EF tooling
remove irrelevant #if NET46 code

nits:

sort dependencies
clean up whitespace
accept VS suggestions

- incorporate aspnet/FrameworkBenchmarks@4830e2fc, "Only Prepare DbCommand once" - enable more scenarios e.g. "--scenarios plaintext" also enables "mvc/plaintext" scenario - optimize `Content-Length` setting in plaintext scenario - remove unused Microsoft.EntityFrameworkCore.Design dependency and EF tooling - remove irrelevant `#if NET46` code nits: - sort dependencies - clean up whitespace - accept VS suggestions

cesarblum

enable more scenarios e.g. "--scenarios plaintext" also enables "mvc/plaintext" scenario

Does this mean when I run the driver with -n plaintext I'll be getting more than one job run?

cesarblum · 2017-05-22T23:52:12Z

src/Benchmarks/Middleware/PlaintextMiddleware.cs

+
+            // HACK: Setting the Content-Length header manually avoids the cost of serializing the int to a string.
+            //       This is instead of: httpContext.Response.ContentLength = payloadLength;
+            response.Headers["Content-Length"] = "13";


What is the perf difference?

I think we want to keep the code in Benchmarks: 19ecf5e

This would be a regression in 2.0 with the add ContentLength to IHeaderDictionary changes; the hack is fixed and no longer needed (and becomes a regression)

🆗 I'll undo this here and fix the TechEmpower code.

You don't want to fix the TechEmpower until it uses 2.0 though.

TechEmpower is being updated to target .NET Core 2.0.0-preview1.

Hooray! 😄

mikeharder · 2017-05-22T23:54:35Z

src/Benchmarks/Configuration/Scenarios.cs

            {
                EnableDefault();
                return 2;
            }

            var props = typeof(Scenarios).GetTypeInfo().DeclaredProperties
-                .Where(p => string.Equals(partialName, "[all]", StringComparison.OrdinalIgnoreCase) || p.Name.StartsWith(partialName, StringComparison.OrdinalIgnoreCase))
+                .Where(p => string.Equals(partialName, "[all]", StringComparison.OrdinalIgnoreCase) || p.Name.IndexOf(partialName, StringComparison.OrdinalIgnoreCase) >= 0)


We want to keep StartsWith: c22b982

If anything I would update TechEmpower to also use StartsWith.

Shoot that's going to break many of the TechEmpower scripts but I understand the "enable the minimum" motivation. ☹️ but 🆗

mikeharder · 2017-05-22T23:56:18Z

src/Benchmarks/Benchmarks.csproj

@@ -17,7 +16,6 @@
    <PackageReference Include="Microsoft.AspNetCore.Server.Kestrel.Https" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.AspNetCore.StaticFiles" Version="$(AspNetCoreVersion)" />
-    <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="$(AspNetCoreVersion)" PrivateAssets="All" />


Do you know why this can/should be removed?

… because databases are automatically created in all cases. No need for dotnet ef database update, migration management, et cetera.

mikeharder · 2017-05-22T23:56:23Z

src/Benchmarks/Benchmarks.csproj

  </ItemGroup>
-
-  <ItemGroup>


Do you know why this can/should be removed?

mikeharder · 2017-05-22T23:57:45Z

src/Benchmarks/Data/DapperDb.cs

@@ -40,7 +40,7 @@ public DapperDb(IRandom random, DbProviderFactory dbProviderFactory, IOptions<Ap
        async Task<World> ReadSingleRow(DbConnection db)
        {
            return await db.QueryFirstOrDefaultAsync<World>(
-                    "SELECT id, randomnumber FROM world WHERE Id = @Id",
+                    "SELECT id, randomnumber FROM world WHERE id = @Id",


I think this was required for compat with a particular DB or driver, but either way it seems better all-lowercase.

mikeharder · 2017-05-23T00:00:10Z

src/Benchmarks/Middleware/PlaintextMiddleware.cs

+
+            // HACK: Setting the Content-Length header manually avoids the cost of serializing the int to a string.
+            //       This is instead of: httpContext.Response.ContentLength = payloadLength;
+            response.Headers["Content-Length"] = "13";


I think we want to keep the code in Benchmarks: 19ecf5e

benaadams · 2017-05-23T00:25:34Z

src/Benchmarks/Middleware/PlaintextMiddleware.cs

+
+            // HACK: Setting the Content-Length header manually avoids the cost of serializing the int to a string.
+            //       This is instead of: httpContext.Response.ContentLength = payloadLength;
+            response.Headers["Content-Length"] = "13";


This would be a regression in 2.0 with the add ContentLength to IHeaderDictionary changes

"React to IHeaderDictionary ContentLength change" #170
"[Perf] Add ContentLength to IHeaderDictionary" aspnet/HttpAbstractions#757
"Use ContentLength as primary data source" aspnet/KestrelHttpServer#1313

- enable more scenarios e.g. "--scenarios plaintext" also enables "mvc/plaintext" scenario - stick with enabling the minimum - optimize `Content-Length` setting in plaintext scenario - wasn't an optimization

dougbu · 2017-05-23T22:06:28Z

fc92496

dnfclas added the cla-already-signed label May 22, 2017

dougbu requested a review from mikeharder May 22, 2017 23:50

cesarblum reviewed May 22, 2017

View reviewed changes

mikeharder requested changes May 23, 2017

View reviewed changes

benaadams suggested changes May 23, 2017

View reviewed changes

Revert two bits

fcb35bc

- enable more scenarios e.g. "--scenarios plaintext" also enables "mvc/plaintext" scenario - stick with enabling the minimum - optimize `Content-Length` setting in plaintext scenario - wasn't an optimization

mikeharder approved these changes May 23, 2017

View reviewed changes

dougbu closed this May 23, 2017

dougbu deleted the dougbu/rationalizations branch May 23, 2017 22:06

Bring over a few changes from the TechEmpower repo #266

Bring over a few changes from the TechEmpower repo #266

Uh oh!

Conversation

dougbu commented May 22, 2017

Uh oh!

cesarblum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented May 23, 2017

Uh oh!

Uh oh!

benaadams May 23, 2017 •

edited

Loading

benaadams May 23, 2017 •

edited

Loading