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

Added measure-elapsed-time example #343

Merged
merged 3 commits into from Oct 26, 2017

Conversation

Projects
None yet
2 participants
@AlexEne
Copy link
Contributor

AlexEne commented Oct 24, 2017

I just explored a bit this repo, read the contribution guidelines, one thing lead to another and I added the measure-elapsed-time example.

fixes #341

@budziq
Copy link
Collaborator

budziq left a comment

Thanks @AlexEne !
Please find some cosmetic suggestions below.

use std::time::{Duration, Instant};
use std::thread;
fn expensive_function() {

This comment has been minimized.

@budziq

budziq Oct 25, 2017

Collaborator

Lets hide the parts of the example that are not directly related to actual measurement with leading #

# use std:thread
# 
# fn expensive_function() {
# thread::sleep(Duration::from_millis(10));
# }
let duration = Instant::now() - start();
```

This is the fastest way of getting some quick and dirty

This comment has been minimized.

@budziq

budziq Oct 25, 2017

Collaborator

I wouldn't exactly say that it's profiling nor benchmarking (there are better and easier ways to benchmark built into rust nightly or available in crates.io for stable), actually I would not like to even suggest that this is even a dirty form of either one of these. Rather just an elapsed time reporting that is also useful in some cases.

I'd suggest to remove or rewrite this last two paragraphs.

This comment has been minimized.

@AlexEne

AlexEne Oct 25, 2017

Author Contributor

Ok, I missed the point a bit since the description was a bit generic. I will do a PR with these fixes later today.

```rust,no_run
let start = Instant::now();
expensive_function();
let duration = Instant::now() - start();

This comment has been minimized.

@budziq

budziq Oct 25, 2017

Collaborator

start() is not callable, this is why the CI complains

This comment has been minimized.

@AlexEne

AlexEne Oct 25, 2017

Author Contributor

Ah, I see, I believed that it has to be a full rust program so it compiles (with main and all that)

Another way to get the time [`Duration`] is as a difference of two
time [`Instant`] objects.

```rust,no_run

This comment has been minimized.

@budziq

budziq Oct 25, 2017

Collaborator

I'd remove this second approach all together.

This comment has been minimized.

@AlexEne

AlexEne Oct 25, 2017

Author Contributor

Done


[![std-badge]][std] [![cat-time-badge]][cat-time]

One way to get the elapsed time is to directly use the method

This comment has been minimized.

@budziq

budziq Oct 25, 2017

Collaborator

Let's reformat the description somewhat to a form like "Measures [time::Instant::elapsed] since [time::Instant::now]"

expensive_function();
let duration = start.elapsed();
println!("Time elapsed in expensive_function() is: {:?}", duration);

This comment has been minimized.

@budziq

budziq Oct 25, 2017

Collaborator

One possible tripwire here is that elapsed does not reset the start in any way.

We can suggest following or just mention the fact in the description.

let start = Instant::now();
expensive_function();
let (start, duration) = (Instant::now(), start.elapsed());
println!("Time elapsed in expensive_function() is: {:?}", duration);
some_other_function();
println!("Time elapsed in some_other_function() is: {:?}", start.elapsed());

This comment has been minimized.

@AlexEne

AlexEne Oct 25, 2017

Author Contributor

I will just mention it in the description since I think this example will be a bit harder to follow and since start is reset before the println!(), makes it a bit imprecise.

@budziq budziq merged commit 57ad48c into rust-lang-nursery:master Oct 26, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Oct 26, 2017

Thanks @AlexEne !

Please also checkout #209 and comment there if you consent to repository re-license to CC0.

@AlexEne

This comment has been minimized.

Copy link
Contributor Author

AlexEne commented Oct 26, 2017

I commented on that thread, I agree with the idea and thanks for all the feedback ;)

@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Oct 26, 2017

Thanks! Not that much feedback was needed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.