-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
appMpower version 1.0 #6614
appMpower version 1.0 #6614
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first commit of a new CSharp framework that is special in the following respect:
- It is natively compiled with the option to disable reflection: this generates the smallest executable
- It uses the ODBC interface instead of the .NET ngpsql library (driven by the reflection free implementation).
Can someone review and if ok approve this as a maintainer as this is a first time contribution ?
Letting CI run and will get around to a review in the next couple of days. @sebastienros @benaadams @lauxjpn don't know if this is something you guys are interested in looking at. |
/cc @roji |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some performance suggestions, but looks good
} | ||
else if (pathStringLength == 5 && pathStringStart == "j") | ||
{ | ||
Json.RenderOne(httpResponse.Headers, httpResponseBody.Writer, new JsonMessage { message = "Hello, World!" }, new JsonMessageSerializer()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new JsonMessageSerializer()
looks idempotent and everything is passed via parameter; so could hoist this class to a readonly static rather than allocating each time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review Ben. This recurring suggestion is a good one. As I am new on github since April and this is my first TechEmpower Benchmark pull request, can you just let me know how I best do the change ? Do I start locally in my appMpowerv1 branch, commit there, push to my forked repository and then do a new pull request to this repository ? Just to be sure: I assume your suggestion is like adding
private readonly static WorldSerializer _worldSerializer = new WorldSerializer();
at the top of HttpApplication.cs and using that one ?
Luc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure: I assume your suggestion is like adding
private readonly static WorldSerializer _worldSerializer = new WorldSerializer();
Yep
Do I start locally in my appMpowerv1 branch, commit there, push to my forked repository and then do a new pull request to this repository ?
If the PR is still open (e.g. this one) you just need to push to same fork and it will automatically update, you don't need to do another pull request.
} | ||
else if (pathStringLength == 3 && pathStringStart == "d") | ||
{ | ||
Json.RenderOne(httpResponse.Headers, httpResponseBody.Writer, await RawDb.LoadSingleQueryRow(), new WorldSerializer()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new WorldSerializer()
also looks idempotent and everything is passed via parameter; so could hoist this class to a readonly static rather than allocating each time?
} | ||
|
||
//Json.RenderMany(httpResponse.Headers, httpResponseBody.Writer, await RawDb.LoadMultipleQueriesRows(count), new WorldSerializer()); | ||
Json.RenderMany(httpResponse.Headers, httpResponseBody.Writer, await RawDb.ReadMultipleRows(count), new WorldSerializer()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
count = 500; | ||
} | ||
|
||
Json.RenderMany(httpResponse.Headers, httpResponseBody.Writer, await RawDb.LoadMultipleUpdatesRows(count), new WorldSerializer()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
@lauxjpn thanks for the ping. Yeah, we discussed in dotnet/aspnetcore#31561 (comment) about using Npgsql. I've done some work to make it reflection-free etc, though I can't say for sure it would work in this scenario. We'll give it a try after this is merged etc. |
This implementation in .NET and Kestrel is natively compiled in a reflection-free way. As the .NET ODBC interface library is written in a reflection-free way, this interface is used to PostgreSQL. In these 2 respects, this framework implementation is unique.