Nswag: Improving performance

[kimbell]

I've been using NSwag for some time now, and have been browsing the generated code on more than one occasion. Today I was made aware of a some work David Fowler is doing to educate the community on best practices around async code. One of his 'bad' examples rang a bell.

Assuming your code generation hasn't undergone major changes recently, NSwag generates code like this:

var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
var result_ = default(AcceptanceReply);
try
{
      result_ = Newtonsoft.Json.JsonConvert.DeserializeObject<AcceptanceReply>(responseData_, _settings.Value);
      return result_;
 }
catch (System.Exception exception_)
{
      throw new SwaggerException("Could not deserialize the response body.", (int)response_.StatusCode, responseData_, headers_, exception_);
}

The whole string is read into memory before it gets deserialized into an object.

Fowler is suggesting this improved approach:
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/Scenarios/Services/PokemonService.cs#L36

public async Task<PokemonData> GetPokemonAsync()
{
      var response = await _client.GetAsync(_url);
      // This is a hack to work around the fact that this JSON api returns text/plain
      response.Content.Headers.ContentType = new MediaTypeHeaderValue("application/json");
      // Use the built in methods to read the response as JSON
      return await response.Content.ReadAsAsync<PokemonData>();
 }

[SebastianStehle]

Just until recently I had the same approach of using the stream to parse the json, but recently we had a big integration projection with lots of external services and we decided to read the full body to memory and log it with all request and response headers. And we had almost no performance decrease.

[kimbell]

I've done the same thing in some of my projects, and for those scenarios it hasn't been a problem.

Since NSwag is a general purpose library, one should try to make as few assumptions as possible. When the advice comes from one of the central ASP.NET Core guys, one should at least consider it.

NSwag is free to stick with the current approach if that makes sense for other reasons, but then it would be helpful to have some note about it.

[SebastianStehle]

True, should be easy to make a PR for that ;)

[Jehoel]

In my hand-written clients I implement two different deserialization methods: one using an intermedia String and the other using JsonTextReader (or just the built-in object deserialization method in HttpClient) wrapped in #if DEBUG so the Release code uses the fast code and debug code lets me see the raw request and response text. Is this an approach that NSwag could use?

[RicoSuter]

@jehoel, sounds good so long as we dont break anything and dont change the public api

[Jehoel]

@RSuter Thank you - I'll prepare a PR sometime this week.

[RicoSuter]

Is JsonTextReader using newtonsoft.json internally? And please dont create a huge pr with lots of changes - better multiple ones. Its very hard to review otherwise. Every line of code has a reason and we need to support all existing scenarios/settings.

[Jehoel]

Is JsonTextReader using newtonsoft.json internally?

JsonTextReader is part of Newtonsoft.Json: https://www.newtonsoft.com/json/help/html/ReadJsonWithJsonTextReader.htm (but rather than using it on a StringReader it can use any TextReader, such as StreamReader).

And please dont create a huge pr with lots of changes - better multiple ones

I'll follow this guidance.

[RicoSuter]

Nice, maybe you also sync with @jehoel here: https://github.com/RSuter/NSwag/issues/1697

[Jehoel]

Nice, maybe you also sync with @Jehoel here: #1697

That's me :)

[RicoSuter]

Ou yes ;-) sorry

[Jehoel]

@RSuter I just emailed you regarding some build issues I'm experiencing.

[RicoSuter]

Maybe outdated: https://github.com/RSuter/NSwag/wiki/SDK-Development

Will reply tomorrow

[RicoSuter]

As a starter, try to open and build NSwag.Min.sln

[Jehoel]

Despite having the .NET Core SDK 2.1 installed, I get this error in VS's Error List when I try to build NSwag.Min.sln:

Severity Code Description Project File Line
Error The current .NET SDK does not support targeting .NET Core 2.1. Either target .NET Core 1.1 or lower, or use a version of the .NET SDK that supports .NET Core 2.1. NSwag.SwaggerGeneration.AspNetCore.Tests C:\Program Files (x86)\Microsoft Visual Studio 2017\MSBuild\Sdks\Microsoft.NET.Sdk\build\Microsoft.NET.TargetFrameworkInference.targets 112

I've filed an issue with the build errors I get from the command-line build here: https://github.com/RSuter/NSwag/issues/1702

[juanperezADD]

Any news? @Jehoel, Have you fixed the compilation errors?

We are also looking for improving the performance by deserializing the server response as a stream instead of a string.

Kind Regards

[Jehoel]

@juanperezADD I was able to implement the change by creating my own .liquid templates and putting them in a directory specified in NSwagStudio. I'll need to polish them up and remove proprietary bits before I can share them here though.

[juanperezADD]

Amazing news. We can't wait to use your code. Thank you in advance.

Kind Regards

[RicoSuter]

If this change is retaining all features and does not introduce breaking changes in the generated public api, I'm really happy to incorporate this into the main NSwag sources...

[Jehoel]

My templates are available at https://github.com/Jehoel/NSwag/tree/dev/better-templates/src/NSwag.CodeGeneration.CSharp/Templates - I rebased them on the current master HEAD on RSuter/NSwag as of a few hours ago.

Warning: they're opinionated and conflict with some design decisions made by the maintainers of NSwag. These template changes do introduce some breaking changes, for example: added exception constructor overloads for enum-based "reason" values, and the string-content of responses is not available as a side-effect of using streams without buffering the whole response as a string (but it does use strings when DEBUG is defined).

Some of the other changes include:

• Using Int32 for status codes instead of strings (why was it using strings in the first place?)
• Importing common namespaces in File.liquid to reduce the verbose System...-spam everywhere.
• Using explicit types instead of var in some key places, especially where it affects readability of code.
  
  
  
  • I'm in the "var is dangerous" school of thought. var has its place where types can only be inferred or when full type names are overly complex, e.g. in deep Linq queries or when dealing with anonymous types, but I feel the explicit typing of variables makes code much easier to read - and because it makes code readable in a simple text-editor. If you need to fire up an IDE to figure out the data-type of a symbol then you're doing it wrong.
  
  
• ...but I'm sure the project admins will revert my opinionated changes if they ever merge my changes :D

I have a short wishlist of my own to make my generated code better - but I haven't dug too deep within NSwag yet:

• I want to output each Client subclass to its own file.
• I want to move more common functionality to methods inherited from a common base class - for example, I'd like virtual ReadObjectResponseAsync<T> to be in a base-class instead of being in each Client class.

If you would like me to submit a PR then I can do that - I haven't yet because I feel my changes wouldn't be well-received by the NSwag maintainers.

[RicoSuter]

Thanks for sharing this.

Using Int32 for status codes instead of strings (why was it using strings in the first place?)

That's mainly because the status codes in Swagger/OpenApi are strings and there are also some special cases which are not ints (e.g. "default" and in OpenAPI, e.g. "2xx"). So that's to avoid compile errors...

Multiple file output
https://github.com/RSuter/NSwag/issues/1398

I want to move more common functionality to methods inherited from a common base class
The idea is to generate code which works out of the box without an additional nuget package or own code. This should be behind a feature flag setting then like other features on the base class...

I think it would be good to create a PR so that we don't forget about your work and then we can look into your changes and discuss...

[Jehoel]

PR created: https://github.com/RSuter/NSwag/pull/1976

[Jehoel]

Regarding those particular Swagger/OpenAPI HTTP status codes - could we use negative signed Int32 values for those?

e.g.

const Int32 HttpStatusDefault =   -1;
const Int32 HttpStatus2xx     = -200;