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

Fix #1829 #1872

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix #1829 #1872

wants to merge 7 commits into from

Conversation

Daksh-10
Copy link

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.

Copy link
Contributor

@Kobzol Kobzol left a 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.

collector/runtime-benchmarks/asynctest/src/main.rs Outdated Show resolved Hide resolved
collector/runtime-benchmarks/asynctest/src/main.rs Outdated Show resolved Hide resolved
collector/runtime-benchmarks/asynctest/src/main.rs Outdated Show resolved Hide resolved
collector/runtime-benchmarks/asynctest/Cargo.toml Outdated Show resolved Hide resolved
@Daksh-10
Copy link
Author

I will try to fix them, thank you for your suggestions :)

Copy link
Contributor

@Kobzol Kobzol left a 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.?

collector/runtime-benchmarks/async/src/main.rs Outdated Show resolved Hide resolved
collector/runtime-benchmarks/async/src/main.rs Outdated Show resolved Hide resolved
collector/runtime-benchmarks/async/src/main.rs Outdated Show resolved Hide resolved
@Daksh-10
Copy link
Author

Yes, I will send the ss for instructions/cycle next time.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 25, 2024

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

@Daksh-10
Copy link
Author

I did the changes you said, but the relative path was not working on my local machine ( if you say, I can change it to "../../data/sherlock.txt.gz" )

and here is the table
image

Copy link
Contributor

@Kobzol Kobzol left a 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 awaits, 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.

collector/runtime-benchmarks/async/Cargo.toml Show resolved Hide resolved
collector/runtime-benchmarks/async/src/main.rs Outdated Show resolved Hide resolved
collector/runtime-benchmarks/async/src/main.rs Outdated Show resolved Hide resolved
collector/runtime-benchmarks/async/src/main.rs Outdated Show resolved Hide resolved
@Daksh-10
Copy link
Author

Daksh-10 commented Mar 26, 2024

There is a problem, I changed my setup to a different computer and then setup the project locally, but when I run the runtime benchmarks it runs for every benchmark, but the one I added (the async one). It gives this error
image

I think the issue is in the configuration of the group (i.e. I used 'cargo new async' to create the folder ) I added, but I don't know how it might be affecting the compilation. Can you help me with this, I gave it some time but wasn't able to find the solution.
Also, I obviously added it in the 'runtime-benchmark' and gave it the benchlib dependency.
image

I also tried putting my async code in an already created benchmark to try it and its working there. That is why, I think the problem is in the configuration. Sorry for a large message!

@Kobzol
Copy link
Contributor

Kobzol commented Mar 26, 2024

Can you compile the project manually, by going into collector/runtime-benchmarks/async and running cargo build?

@Daksh-10
Copy link
Author

Daksh-10 commented Mar 26, 2024

Can you compile the project manually, by going into collector/runtime-benchmarks/async and running cargo build?

I build it and now its running, should have tried this lol, but why is it running now??
Are the following 'compiling' different ...
image

... from this compiling
image

@Kobzol
Copy link
Contributor

Kobzol commented Mar 26, 2024

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 cargo build, cargo generated it.

@Daksh-10
Copy link
Author

image

The stddev got low again.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 27, 2024

Okay, the structure of the code looks good now! I wonder if we can somehow find more ways of actually benchmarking async though.

I measured the code, and this line: let sherlock_text = String::from_utf8(decompress_file(TEXT_SHERLOCK)).expect("Invalid UTF-8"); takes ~30ms, while this line: let total_char = total_char_count(sherlock_text).await; takes ~6ms. Therefore, this benchmark is essentially now benchmarking gzip decompression 😆

I would suggest moving let sherlock_text = String::from_utf8(decompress_file(TEXT_SHERLOCK)).expect("Invalid UTF-8"); to the preparation function (or even to main), and just passing the decompressed &str input to total_char_count directly.

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 await (which essentially happens here) also might be useful. So I don't have an issue with just keeping the simple character count.

@Daksh-10
Copy link
Author

LOL, I didn't know that we can measure execution time of parts of code. I will think of more ways for async.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 27, 2024

Well, there's no builtin support for it, I just wrapped these two lines with Instant::now() etc. 😆

@Daksh-10
Copy link
Author

Daksh-10 commented Apr 7, 2024

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.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 7, 2024

Yeah, let's just increase the work done within async for now.

@Daksh-10
Copy link
Author

Updated results
image

@Kobzol
Copy link
Contributor

Kobzol commented Apr 15, 2024

I checked the code, and I'm relatively sure that the await is optimized away completely, and the code doesn't even yield into the tokio runtime. That happens because the await-ed future (line_char_count) is executed eagerly, and if it does not block (return Poll::Pending), so it will basically execute synchronously.

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 :)

@Daksh-10
Copy link
Author

Daksh-10 commented Apr 15, 2024

I did thought that line_char_count will take very very less time. Hence, executing eagerly (without switching to waiting and then to running state like in the Round Robin Algorithm, if I am thinking correctly) , therefore, executing synchronously. I think we need a function which takes a considerable about of time,so I will try to look up some string manipulation questions :)

Also working on system level is tough, but interesting. I didn't understand what this code is, but I got an overview.

@Daksh-10
Copy link
Author

Daksh-10 commented Apr 21, 2024

image
Here, the stddev for instruction is high. According to me, that is a positive as number of instructions executed at any given time for async depends on I/O operations resulting in oscillation of waiting and running states. Should it be low for async??

@Kobzol
Copy link
Contributor

Kobzol commented Jul 12, 2024

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 async.

@Daksh-10
Copy link
Author

Daksh-10 commented Jul 13, 2024

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.
https://github.com/pkolaczk/async-runtimes-benchmarks , this repository has async benchmark for different languages, we can use this approach too :)

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.

2 participants