-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add zerg, fix Unhinged content length calculation #10815
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
Draft
MDA2AV
wants to merge
7
commits into
TechEmpower:master
Choose a base branch
from
MDA2AV:add-zerg-fix-unhinged
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,068
−106
Draft
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e1cde90
Add zerg, fix Unhinged content length calculation
MDA2AV e880182
change unhinged classification to platform
MDA2AV 78e4950
delete not necessary files
MDA2AV f087bd5
fix reactor count
MDA2AV a31697f
update to net10
MDA2AV 898a0a4
hardcore worker count for unhinged 52
MDA2AV c128ddd
hardcode plaintext CL
MDA2AV File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| { | ||
| "framework": "zerg", | ||
| "maintainers": ["MDA2AV"], | ||
| "tests": [ | ||
| { | ||
| "default": { | ||
| "plaintext_url": "/plaintext", | ||
| "json_url": "/json", | ||
| "port": 8080, | ||
| "approach": "Realistic", | ||
| "classification": "Platform", | ||
| "database": "None", | ||
| "framework": "zerg", | ||
| "language": "C#", | ||
| "orm": "None", | ||
| "platform": ".NET", | ||
| "webserver": "zerg", | ||
| "os": "Linux", | ||
| "database_os": "Linux", | ||
| "display_name": "zerg", | ||
| "notes": "" | ||
| }, | ||
| "aot": { | ||
| "plaintext_url": "/plaintext", | ||
| "json_url": "/json", | ||
| "port": 8080, | ||
| "approach": "Realistic", | ||
| "classification": "Platform", | ||
| "database": "None", | ||
| "framework": "zerg", | ||
| "language": "C#", | ||
| "orm": "None", | ||
| "platform": ".NET", | ||
| "webserver": "zerg", | ||
| "os": "Linux", | ||
| "database_os": "Linux", | ||
| "display_name": "zerg", | ||
| "notes": "" | ||
| } | ||
| } | ||
| ] | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 a truly bizarre way to calculate the content-length of a response, and I don't think it really matches the spirit of what we're trying to get at with the "Realistic" classification. A truly "Realistic" approach to all tests other than
plaintextwould be not writing the headers at all and leveraging the framework to do that work.It looks like you're writing 2 spaces in the content-length header, writing the body, then overwriting those two spaces with the "tens" and "ones" places that you get from the bytes committed. It's clever, but I am left asking the question of why this is necessary and whether the author intends end-users (e.g., developers) to perform this exercise on a realistic project.
I'm not going to accept this PR as-is. For one, this does not generalize to payloads over 100 bytes. That said, I went in search of better examples of "Realistic" implementations and found essentially everyone computing and specifying their own content-length (which, again, I think is an absurd expectation for end-users but whatever), so I'll say this: if you rearrange this to serialize the json message before constructing the headers, and use the length of that serialization in the header, then send the entire response, then I will consider it "Realistic" alongside its peers who do the same. Looking at spring as a simple example of what I mean.
Personally, I think it should be more like Gemini which simply calls the framework's
jsonmethod and allows that to compute and set content length as well as other headers, but I'm biased.Uh oh!
There was an error while loading. Please reload this page.
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.
I have to indeed agree with you that it is somewhat of a fishy implementation.. but because it only supports content length up to 99. That can be fixed easily by simply leaving more spaces in the Content-Length header, or to be more fair, leave 10 zeroes like Content-Length: 0000000000\r\n, left trailing zeroes are (surprisingly) valid.
Again, I appreciate you guys actually reviewing the entries and I am not here trying to jeopardize anything, in fact I believe that every framework should work as you mentioned - no hardcoded or manually writing headers and would 100% love if that is the only acceptable format, however since it isn't I am forced to do this to be able to achieve the same or higher throughput as other frameworks that also do it.
So, this is a little bit unfair and let me explain why, I built both the epoll and io_uring engines from scratch to leverage low level access and manipulation on the write buffers, otherwise it would not be possible to do this as we need the exact pointer location on the write buffer even when flushing multiple responses at once in pipelined case.
Now, I also have to agree that for a user to have to do write such code it makes no sense so here is my proposal - I'll add a generalized helper to the framework that hides this mechanism from the user so that any length json is supported and user does not have to care about low level details, also this is marked as a platform framework so "(not actually a framework at all)", this is an engine other frameworks use and this entry serves as a benchmark to compare the engine performance without framework overhead.
Again, thanks for the time and effort on this
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.
I get that, but if you want to get it merged in as-is you'll have to mark it "Stripped". It will still be shown in the results, but hidden by default.
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.
I will adjust this to let the framework calculate the content-length as other frameworks