Skip to content
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

plaintext bench #4

Closed
wants to merge 1 commit into from
Closed

Conversation

zah
Copy link
Contributor

@zah zah commented Jun 18, 2018

@nitely has worked on a benchmark for asyncdispatch2, aiming to compare it to the older asyncdispatch and also to some well-known fast frameworks in other languages.

I'm creating this pull request, so we can provide feedback and eventually merge the code.

@zah zah requested a review from cheatfate June 18, 2018 15:49
@zah
Copy link
Contributor Author

zah commented Jun 18, 2018

Thanks Esteban,

My expectation was that the TechEmperor benchmark has published their own testing tool. Can we get a comparison of running this tool with your servers, so we can compare Nim's performance now to that of the best results in other languages? You'll have to spend a little bit of time in getting some of these other implementations up and running.

Sorry for not explaining my expectations well enough upfront. I've re-read my email now and I realize that my wording was not very clear.

@@ -0,0 +1,55 @@
# Benchmarks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you need to add bench to the skipDirs list of the nimble package. Otherwise nimble check will complain that you have violated the rules for allowed package structure.

Alternatively, "bench" can put as a sub-directory inside "tests"

@nitely
Copy link

nitely commented Jun 18, 2018

My expectation was that the TechEmperor benchmark has published their own testing tool.

Indeed they have. It just wrk with a custom lua script to allow pipelining. Setting up the frameworks at the top (rust, go, c++) won't be a problem.

@zah
Copy link
Contributor Author

zah commented Jun 18, 2018

asyncdispatch2 doesn't support multi-threaded servers yet, so please try to see if you can also force these other frameworks in single-threaded mode. We'll eventually add multi-threading support here, so we'll need to repeat the tests in the future. Some instructions or scripts for this will be appreciated.

@nitely
Copy link

nitely commented Jun 18, 2018

I'll do that. But I wonder if multi-threading will be faster than multiple processes + REUSEPORT, I guess I can check how multi-process performs too.

@cheatfate
Copy link
Collaborator

First of all thanks for your work @nitely.
Just a remark:
For asyncnet version you not using procedure readHeaders() which internally performs one more allocation for asyncdispatch2.

Also i'm pretty impressed in 57% of boost in speed for single-threaded version, so i think we will have even more boost for multi-threaded version.

@nitely
Copy link

nitely commented Jun 18, 2018

Also i'm pretty impressed in 57% of boost in speed for single-threaded version

I forgot to update the asyncnet benchmark after I optimized the server code. It's closer to 60k requests/s now.

@cheatfate btw, I get a Error: unhandled exception: (24) Too many open files [TransportOsError] after running wrk a few times. It seems asyncdispatch2 is not closing the tansport socket.

@cheatfate
Copy link
Collaborator

Could you please show me optimized version? because asyncdispatch2 can load all headers in one step too (next step of optimization) and also uses unnecessary readHeaders().

I will check file descriptors leak.

@cheatfate
Copy link
Collaborator

But i can't find any descriptors leakage, but found a problem inside of your code.
server.join() is waiting for somebody to call server.close().

So to stop and close server properly you need to use https://github.com/status-im/nim-asyncdispatch2/blob/master/asyncdispatch2/asyncloop.nim#L521 this function to handle CTRL+C. And in handler you can call stop/close sequence.

@nitely
Copy link

nitely commented Jun 18, 2018

It seems atEof never returns true (even after the client has closed the connection).

server.join() is waiting for somebody to call server.close()

I never stop the server so that shouldn't be an issue.

@cheatfate
Copy link
Collaborator

@nitely you will never get EOF if you will not perform read operation on closed socket, in such way EOF marker will be set.

@nitely
Copy link

nitely commented Jun 18, 2018

It works when I call readLine instead of readUntil. I'll dig the issue later.

@cheatfate
Copy link
Collaborator

@nitely i have found a problem, leakage happens because readUntil() generated exception IncompleteError, and because tasks are started without asyncCheck this exceptions got hidden.

@nitely
Copy link

nitely commented Jun 18, 2018

@cheatfate yeah, that's it. I also found the exception is being swallowed 😄

@cheatfate
Copy link
Collaborator

@nitely what do you think about this benchmark https://gist.github.com/cheatfate/2a46181ab03f1a2dc2c943b3e1a56ca5

cheatfate added a commit that referenced this pull request Jan 21, 2021
cheatfate added a commit that referenced this pull request Feb 10, 2021
cheatfate added a commit that referenced this pull request Feb 10, 2021
zah pushed a commit that referenced this pull request Feb 18, 2021
@arnetheduck
Copy link
Member

This PR would need a full rewrite to stay relevant - closing since there has been no activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants