I've posted a couple of benchmarks involving D previously and received complaints that the D implementation was ugly and un-idiomatic, so I thought I'd seek code review before posting the next one. The code is at https://github.com/logicchains/ParticleBench/blob/master/D.d; it's an OpenGL particle animation, and my D code is essentially just a direct port of the C implementation. Note however that, as the animation is gpu-bound, there's not much scope for optimisation of the code (in hindsight, it was a pretty poor topic for a benchmark, but hindsight always sees farthest and whatnot).

On Friday, 13 December 2013 at 12:41:53 UTC, logicchains wrote:
> I've posted a couple of benchmarks involving D previously and received complaints that the D implementation was ugly and un-idiomatic, so I thought I'd seek code review before posting the next one. The code is at https://github.com/logicchains/ParticleBench/blob/master/D.d; it's an OpenGL particle animation, and my D code is essentially just a direct port of the C implementation. Note however that, as the animation is gpu-bound, there's not much scope for optimisation of the code (in hindsight, it was a pretty poor topic for a benchmark, but hindsight always sees farthest and whatnot).

You have a lot of global variables of same type. With very similar default values.
Example windX and runTmr. They are both doubles.

Perhaps an alternative way to write it is like this:
double
 windX = 0,
 runTmr = 0;

You also have a lot of enums that like WIDTH and HEIGHT that could be transformed into a single enum.
e.g.
enum int WIDTH = 800;
enum int HEIGHT = 600;

Would become:
enum : int {
 WIDTH = 800,
 HEIGHT = 600
}

Adds a couple extra lines but hey when you got 20 odd values and repeating the type it kinda looks ugly.

Nothing really huge to say. I would say not use e.g. Derelict for bindings but when you're not timing that then no point.

Also check your formatting currently its not consistent. There is some brackets with and without spaces before / after.

Otherwise not too badly done.

"Rikki Cattermole" <alphaglosined@gmail.com> wrote in message news:otjpativnfoecwjqethp@forum.dlang.org...
>
> You have a lot of global variables of same type. With very similar default
> values.
> Example windX and runTmr. They are both doubles.
>
> Perhaps an alternative way to write it is like this:
> double
>  windX = 0,
>  runTmr = 0;
>
> You also have a lot of enums that like WIDTH and HEIGHT that could be
> transformed into a single enum.
> e.g.
> enum int WIDTH = 800;
> enum int HEIGHT = 600;
>
> Would become:
> enum : int {
>  WIDTH = 800,
>  HEIGHT = 600
> }
>
> Adds a couple extra lines but hey when you got 20 odd values and repeating the type it kinda looks ugly.
>

I would not consider either of those an improvement.

On Friday, 13 December 2013 at 13:34:07 UTC, Daniel Murphy wrote:
> "Rikki Cattermole" <alphaglosined@gmail.com> wrote in message
> news:otjpativnfoecwjqethp@forum.dlang.org...
>>
>> You have a lot of global variables of same type. With very similar default values.
>> Example windX and runTmr. They are both doubles.
>>
>> Perhaps an alternative way to write it is like this:
>> double
>>  windX = 0,
>>  runTmr = 0;
>>
>> You also have a lot of enums that like WIDTH and HEIGHT that could be transformed into a single enum.
>> e.g.
>> enum int WIDTH = 800;
>> enum int HEIGHT = 600;
>>
>> Would become:
>> enum : int {
>>  WIDTH = 800,
>>  HEIGHT = 600
>> }
>>
>> Adds a couple extra lines but hey when you got 20 odd values and repeating the type it kinda looks ugly.
>>
>
> I would not consider either of those an improvement.

Depends on style I guess. To me its less to read so = good.

On Friday, 13 December 2013 at 13:24:43 UTC, Rikki Cattermole wrote:
> Perhaps an alternative way to write it is like this:
> double
>  windX = 0,
>  runTmr = 0;
>
> You also have a lot of enums that like WIDTH and HEIGHT that could be transformed into a single enum.
> e.g.
> enum int WIDTH = 800;
> enum int HEIGHT = 600;
>
> Would become:
> enum : int {
>  WIDTH = 800,
>  HEIGHT = 600
> }

I didn't realise D could do that; I've updated the code to use that style of variable declaration. It's interesting to be able to declare two arrays at once like so:

double[RUNNING_TIME * 1000]
    frames,
    gpuTimes;

Is there some way to initialise multiple values to zero without writing the zero more than once, so that the following only needs one zero:

double
    windX = 0,
    runTmr = 0;

I am timing, there just isn't much different in results between the different implementations. Why would you say not to use Derelict for bindings?

On Friday, 13 December 2013 at 12:41:53 UTC, logicchains wrote:
> I've posted a couple of benchmarks involving D previously and received complaints that the D implementation was ugly and un-idiomatic, so I thought I'd seek code review before posting the next one. The code is at https://github.com/logicchains/ParticleBench/blob/master/D.d; it's an OpenGL particle animation, and my D code is essentially just a direct port of the C implementation. Note however that, as the animation is gpu-bound, there's not much scope for optimisation of the code (in hindsight, it was a pretty poor topic for a benchmark, but hindsight always sees farthest and whatnot).

Use std.algorithm instead of for-loops for sum calculation:

sum = reduce!"a + b"(0, frames);

I don't think you can do much about it. Most code is just OpenGL API boilerplate or game logic.

On 2013-12-13 14:47, Rikki Cattermole wrote:

> Depends on style I guess. To me its less to read so = good.

It's two additional lines == more to read :)

-- 
/Jacob Carlborg

On Friday, 13 December 2013 at 13:34:07 UTC, Daniel Murphy wrote:
> I would not consider either of those an improvement.

Longer code with less data duplication is better than shorter one with copy-paste.

Am 13.12.2013 14:56, schrieb logicchains:
> I didn't realise D could do that; I've updated the code to use
> that style of variable declaration. It's interesting to be able
> to declare two arrays at once like so:

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

On 13 December 2013 22:41, logicchains <jonathan.t.barnard@gmail.com> wrote:

> I've posted a couple of benchmarks involving D previously and received complaints that the D implementation was ugly and un-idiomatic, so I thought I'd seek code review before posting the next one. The code is at https://github.com/logicchains/ParticleBench/blob/master/D.d; it's an OpenGL particle animation, and my D code is essentially just a direct port of the C implementation. Note however that, as the animation is gpu-bound, there's not much scope for optimisation of the code (in hindsight, it was a pretty poor topic for a benchmark, but hindsight always sees farthest and whatnot).
>

Is it idiomatic to use egyptian braces in D? I've never seen D code written this way... looks like Java.

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

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?

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

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

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.

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.

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.