-
Notifications
You must be signed in to change notification settings - Fork 149
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
Fix #1829 #1872
base: master
Are you sure you want to change the base?
Fix #1829 #1872
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.
Hi, thank you for working on this! I left some comments.
I will try to fix them, thank you for your suggestions :) |
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! I left a few more comments.
Could you please run the benchmark locally and post the instruction count/cycle results etc.?
Yes, I will send the ss for instructions/cycle next time. |
By the way, if you want to update to the newest version of master (and/or force push), you can do that like this: $ git fetch origin master:master
$ git rebase master
$ git push --force |
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, I left more comments.
I went through a profile of the benchmark (using samply
), and it looks like it mostly stresses the decompression and memory movement. OTOH, there are 15 thousand lines in the file, so it should execute at least that number of await
s, so if something went wrong there, we could perhaps observe it in the results.
The standard deviation of the executed instructions is remarkably low, which is great. I think that for a start, it looks good.
Can you compile the project manually, by going into |
I don't know what was causing your previous error. Probably it was a missing Cargo.lock file (that you haven't committed yet in this PR)? When you executed |
Okay, the structure of the code looks good now! I wonder if we can somehow find more ways of actually benchmarking I measured the code, and this line: I would suggest moving And if you can think of something, maybe add some more async operations :) Creating synthetic benchmarks like this is quite tricky. That being said, having one benchmark where we perform very little operations per each |
LOL, I didn't know that we can measure execution time of parts of code. I will think of more ways for async. |
Well, there's no builtin support for it, I just wrapped these two lines with |
I thought of some more ways like, fetching info from public resources, sending thousands of requests to dummy servers, but these might have issues of internet speed or shutting down of resources in future. So, probably should I just increase the iterations for async word_count function?? Because it works. |
Yeah, let's just increase the work done within async for now. |
I checked the code, and I'm relatively sure that the The current version of the code thus doesn't really benchmark anything inside of the tokio runtime. We probably need a better benchmark, sigh. Benchmarking is hard :) |
I did thought that Also working on system level is tough, but interesting. I didn't understand what this code is, but I got an overview. |
Hi, sorry for taking so long, this PR fell through the cracks. Given our experience with very weird benchmark results in this PR, I think that it would be better to add an async benchmark adapted from some real code, rather than such a trivial microbenchmark. Our current runtime benchmark suite is not really prepared for such a quickly running code (this is also a problem for some other benchmarks in the suite), and from what we have seen so far, this benchmark was measuring a lot of things, but not the overhead of |
What we can do is one choice from HTTP request to free APIs, I/O operations on file(read and write), some basic function executed asynchronously or all of them. |
In #1829,
I added a runtime benchmark for async/await, it reads two files asynchronously for 1000 times and provides the duration for which the code works.