search

IO500 / io500

IO500 Storage Benchmark source code
MIT License
97 stars 31 forks source link

Fix stonewall performance calculation (unused so far). #18

Closed JulianKunkel closed 3 years ago

JulianKunkel commented 3 years ago

The current performance calculation that is output for IOR when hitting the stonewall is incorrect.

gflofst commented 3 years ago

Can you describe how the calculation was wrong and how it is changing? How much will this affect the rankings? It looks like not much.

adilger commented 3 years ago

Jay, it looks like it may be avoiding potential loss in accuracy due to integer truncation in the math (e.g. if p->stonewall_avg_data_accessed is small compared to p->time then the result may lose precision or be zero). I don't recall off the top of my head the C type promotion details to know if the intitial "long long / double" is done in long long or double type. To avoid any confusion it would be better to cast (double)p->stonewall_avg_data_accessed at the start, and then the order of calculations won't really matter.

That said, I don't think this change affects the results in any way, since it is just a statistic that is printed by the IOR run.

gflofst commented 3 years ago

It would promote to double for sure. Ok. That is what I wanted to see in the bug report. Nothing worse that "I fixed a problem" sort of problem descriptions. No idea what to look at when reviewing it. :-)

JulianKunkel commented 3 years ago

Hi, it doesn't affect the ranking at all as we are not using this value at the moment which is the performance at the moment the stonewall is hit. The problem here was that it was divided by time instead of stonewall time. Also inside ior the avg wasn't divided by nproc. This is fixed in a PR to IOR.