Skip to content

Add Jil JSON test #36

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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
55 changes: 55 additions & 0 deletions src/Benchmarks/JsonJilMiddleware.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// 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.

using System;
using System.IO;
using System.Text;
using System.Threading.Tasks;
using Microsoft.AspNet.Builder;
using Microsoft.AspNet.Http;
using Jil;

namespace Benchmarks
{
public class JsonJilMiddleware
{
private static readonly Task _done = Task.FromResult(0);
private static readonly PathString _path = new PathString("/json/jil");

private readonly RequestDelegate _next;

public JsonJilMiddleware(RequestDelegate next)
{
_next = next;
}

public Task Invoke(HttpContext httpContext)
{
// We check Ordinal explicitly first because it's faster than OrdinalIgnoreCase
if (httpContext.Request.Path.StartsWithSegments(_path, StringComparison.Ordinal) ||
httpContext.Request.Path.StartsWithSegments(_path, StringComparison.OrdinalIgnoreCase))
{
httpContext.Response.StatusCode = 200;
httpContext.Response.ContentType = "application/json";
httpContext.Response.ContentLength = 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is copied from JsonMiddleware.cs, I note - these hard-coded 30s smell a bit like cheating, though, no? (in both cases; I'm not limiting this to #36)

Copy link
Member

Choose a reason for hiding this comment

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

Cheating why?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to go fast you should be doing httpContext.Response.Headers["Content-Length"] = "30"; as is done in the plaintext middleware

As the numeric form does a bunch of conversions

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidfowl it just seems odd to hard-code the content length, rather than either ask the serializer what it will be, or (more commonly) buffer the output, write the length of the buffer, then write the buffer. I realize the intent is to remove the buffer, but .... meh, maybe I'm over-thinking it.

Copy link
Author

Choose a reason for hiding this comment

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

My only goal was to put the two on equal footing - I'm all for changing both to be more realistic, though.


using (var sw = new StreamWriter(httpContext.Response.Body, Encoding.UTF8, bufferSize: 30))
{
JSON.Serialize(new { message = "Hello, World!" }, sw);
}

return _done;
}

return _next(httpContext);
}
}

public static class JsonJilMiddlewareExtensions
{
public static IApplicationBuilder UseJilJson(this IApplicationBuilder builder)
{
return builder.UseMiddleware<JsonJilMiddleware>();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@

namespace Benchmarks
{
public class JsonMiddleware
public class JsonNewtonsoftMiddleware
{
private static readonly Task _done = Task.FromResult(0);
private static readonly PathString _path = new PathString("/json");
private static readonly PathString _path = new PathString("/json/newtonsoft");
private static readonly JsonSerializer _json = new JsonSerializer();

private readonly RequestDelegate _next;

public JsonMiddleware(RequestDelegate next)
public JsonNewtonsoftMiddleware(RequestDelegate next)
{
_next = next;
}
Expand All @@ -46,11 +46,11 @@ public Task Invoke(HttpContext httpContext)
}
}

public static class JsonMiddlewareExtensions
public static class JsonNewtonsoftMiddlewareExtensions
{
public static IApplicationBuilder UseJson(this IApplicationBuilder builder)
public static IApplicationBuilder UseNewtonsoftJson(this IApplicationBuilder builder)
{
return builder.UseMiddleware<JsonMiddleware>();
return builder.UseMiddleware<JsonNewtonsoftMiddleware>();
}
}
}
3 changes: 2 additions & 1 deletion src/Benchmarks/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ public void Configure(IApplicationBuilder app)
{
app.UseErrorHandler();
app.UsePlainText();
app.UseJson();
app.UseNewtonsoftJson();
app.UseJilJson();

if (StartupOptions.EnableDbTests)
{
Expand Down
3 changes: 2 additions & 1 deletion src/Benchmarks/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"Microsoft.AspNet.Server.WebListener": "1.0.0-*",
"Microsoft.AspNet.StaticFiles": "1.0.0-*",
"Microsoft.Extensions.WebEncoders": "1.0.0-*",
"Newtonsoft.Json": "7.0.1"
"Newtonsoft.Json": "7.0.1",
"Jil": "2.13.0-*"
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 use a specific version here?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah sure - I wasn't sure what the desire was since Jil's in beta for a bit while some missing corefx bits are added (waiting on https://github.com/dotnet/corefx/issues/4543 for another rev). I'll update to be specific though.

Edit: isn't this actually inconsistent with all the other libs? e.g. this is how @DamianEdwards did Dapper as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything except Newtonsoft.Json uses wildcard... just sayin'

There are pros and cons of either approach; it saved me having to issue a PR when I last rev'd "dapper" ;p

},

"commands": {
Expand Down