D benchmark code review

[logicchains]

On Friday, 13 December 2013 at 15:58:43 UTC, dennis luehring 
wrote:
> please revert - it looks terrible newbie like, no one except 
> Rikki writes it like this
>
> it makes absolutely no sense to pseudo-scope these enums and 
> variable declarations, they do not share anything except the 
> type AND that is not enough for pseudo-scope

I thought it seemed useful in that it'd make it easier to e.g. 
change all those doubles to floats. If it's generally considered 
unidiomatic D, however, then I can revert it.

On Friday, 13 December 2013 at 16:06:10 UTC, Manu wrote:
> Is it idiomatic to use egyptian braces in D? I've never seen D 
> code written
> this way... looks like Java.

It's just the style I've been using in the benchmark; even the 
lisp implementations use it. I think it's due to the influence of 
programming in Go, where that style is mandated (the compiler 
throws an error if you put a newline before the {).

> You have a lot of globals, and they're all TLS. That seems 
> inefficient, and
> potentially incorrect if there were threads.

You're right; I just copied the C code, I didn't consider that it 
being TLS in D might reduce efficiency. I don't mind it not being 
threadsafe, as I don't have any intention to parallelise the 
benchmark, due to most of the time being spent on rendering that 
can't be parallelised.

> Why do you build the vertex array from literal data at runtime? 
> Why not
> just initialise the array?
> It's a static array, so it's not allocated at runtime, so it 
> shouldn't be
> any different. You'd probably save a lot of LOC to initialise 
> the array,
> perhaps better for clarity. It should probably also be 
> immutable?

I left initialising the array to runtime for clarity; I think 
it's easier to read that way than if it's just declared as a 
really long array literal. I'd be happy to populate the array at 
compile time though, if there's an easy way to change the 
initialisation functions to run at compile time. Making it 
immutable is a good idea, although it wouldn't help performance 
as the array is only read once, when it's being fed into the 
vertex buffer.

> Those integer for loops might be nicer as: foreach(i; 0 .. n)

Updated it to use that. I was wondering how to do foreach over a 
numerical range in D; now I know, thanks.

> I'm not personally in the camp that agrees:
>   double
>     x = 0,
>     y = 10,
>     z = 15;
> is a good thing... but each to their own :)

I just assumed it'd be better as it allows for easier refactoring 
(only need to change 'double' one).

> Why are you using 'double' in realtime software? That's weird 
> isn't it?
> Granted, it will make no difference on x86, but I'd never 
> personally do
> this for portability reasons.

Because in my previous benchmarks I've found it performs no 
differently from float on this system, and because it's a direct 
port of the C implementation, where I paranoidly used double in 
order to avoid precision errors if it's ever run on a system 
where float is 16 bit. It also makes for fairer comparison with 
the Go code, which uses 64 bit floats as the standard maths 
library doesn't support f32s.

> But generally looks fine. Any opengl code is naturally going to 
> look C-ish
> by nature. And yes, as you said, it's not really a very good 
> benchmark,
> since you're really benchmarking the GPU (and the legacy 
> fallback
> non-shader pipeline at that) ;) .. The mainloop barely does 
> anything, and
> has very few iterations.

Yep. I tried with shaders but it didn't really improve the speed 
at all, as most of the time's spent rendering, so I'm using the 
legacy pipeline for convenience.

> Nitpick: I think properness would have you update the velocity 
> before the
> position, or you'll always run 1 frame behind.
> There are a lot more really minor performance hazards that I'd 
> never let
> slip if they were invoked in hot loops, but they won't make any 
> difference
> here.

You're right; I've reordered doWind, checkColls and movePts. 
Thanks for all the tips!

On Friday, 13 December 2013 at 14:45:49 UTC, qznc wrote:
> Use std.algorithm instead of for-loops for sum calculation:
>
> sum = reduce!"a + b"(0, frames);

I tried 'auto sum = reduce!"a + b"(0, frames);' but I get an 
error:

algorithm.d(782): Error: cannot implicitly convert expression 
(binaryFun(result._expand_field_0, elem)) of type double to int
std/conv.d(3606): Error: cannot implicitly convert expression 
(_param_1) of type double to int
algorithm.d(794): Error: template instance std.conv.emplace!(int, 
double) error instantiating
D.d(308):        instantiated from here: reduce!(int, 
double[25000LU])