std.benchmark is in reviewable state

[Andrei Alexandrescu]

On 10/3/11 8:59 AM, Jens Mueller wrote:
[snip]

Thanks for this rich feedback. I'll act on most or all points. A few 
notes follow.

> 1. I'm not that convinced that prepending benchmark_ to a function is
> that nice even though it's simple. But following this idea I could
> remove all my unittest blocks and add functions starting with test_ and
> write a module std.test in same fashion. If this approach is considered
> the way to go we can remove unittest altogether and solve testing
> entirely in a library. After all it looks less elegant to me but I admit
> that I see no other way and it is the easiest solution after all. I just
> want to stress that when seeing the code I questioned myself whether
> having unittest in the language is nice to have.

std.benchmark uses relatively new reflection features that weren't 
available until recently. Adding unittest now would be difficult to 
justify, but at the time unittest was a good feature.

> 2. When benchmarking code you often investigate the algorithm's
> properties regarding its scalability.

Yah, that definitely needs to be looked at. I think I need to add some 
support beyond mere documentation.

> 3.
> (r[i] / 10_000_000.).to!("nsecs", int)
> should be written as
> (r[i] / 10_000_000).nsecs
>
> Same for msecs and similar.

Noted.

> 4. Test results should be exposed to the caller.
> Usually, it is enough to write the results to the console. But you may
> want to post process the results. Being it either graphically or to
> output XML for whatever reason.
> I think it would be beneficial to add this flexibility and separate the
> benchmarking logic from its generating the output.
> It might be useful to add the used CPU using std.cpuid to the output.

We should decouple collection from formatting. However, this complicates 
matters because we'd need to expose the benchmark results structure. 
I'll think of it.

> 5. In the first example I completely missed that it is not "relative
> calls". I thought this was one term even though I didn't know what it
> should mean. Later I figured that there is an empty column called
> relative.

There are two spaces there, I should add three.

> 6. Wrong numbers in documentation
> The examples in the documentation have wrong numbers. In the end one
> should pay special attention that the documentation is consistent. I
> think one cannot automate this.

Not sure how this can be addressed at all (or maybe I don't understand). 
The examples show real numbers collected from real runs on my laptop. I 
don't think one may expect to reproduce them with reasonable accuracy 
because they depend on a lot of imponderables.

> There are also rounding errors. That means when I try to compute these
> numbers I get different in the first digit after the decimal point. I
> wonder why this is. Even though the measurement may be noisy the
> computed numbers should be more precise I think.

Where are calculations mistaken? Indeed I see e.g. that append built-in 
has 83ns/call and 11.96M calls/s. Reverting the first with a 
"calculator" program yields 12.04 Mcalls/s, and reverting the second 
yields 83.61ns/call. I'll see what can be done.

> 7. Changing units in the output.
> One benchmark report may use different units in its output. The one in
> the example uses microseconds and nanoseconds. This makes comparison
> more difficult and should be avoided (at least for groups). It is
> error-prone when analyzing the numbers.

Problem is space and wildly varying numbers. I initially had exponential 
notation for everything, but it was super ugly and actually more 
difficult to navigate. That's why I added the compact notation. I think 
we'll need to put up with it.

> 8. I prefer writing std.benchmark.pause() over benchmarkPause() and
> std.benchmark.resume() over benchmarkResume().

OK.

> 10. The file reading/writing benchmark should be improved. Since the
> file should be deleted. The file writing benchmark should be more like:
> void benchmark_fileWrite()
> {
>      benchmarkPause();
>      auto f = File.tmpfile();
>      benchmarkResume();
>
>      f.writeln("hello, world!");
> }
>
> But this also measures the deletion of the file.

If you don't want to measure std.file.write but instead only the time 
spent in writeln, his should work and measure only the time spent in 
writing to the file:

void benchmark_fileWrite()
{
      benchmarkPause();
      auto f = File.tmpfile();
      benchmarkResume();
      f.writeln("hello, world!");
      benchmarkPause();
}

I haven't yet tested the case when a functions leaves the benchmark paused.

> I have no idea how to fix the benchmark for reading a file. But I
> believe it involves using mkstemp on POSIX and similar on Windows.
> mkstemp should be added to std.file in the long run.

Why is there a need to fix it? The benchmark measures the performance of 
std.file.read soup to nuts.

In related news, I got word from a colleagues that I should also use 
times() (http://linux.die.net/man/2/times) to distinguish time spent in 
user space vs. system calls.  This is because some functions must issue 
certain system calls in any case and the user is interested in how much 
they can improve the situation by optimizing their end of the deal. This 
would apply e.g. to file I/O where the overhead of formatting etc. would 
become easier to distinguish.


Andrei