xtensor-stack / xtensor

C++ tensors with broadcasting and lazy computing
BSD 3-Clause "New" or "Revised" License
3.33k stars 398 forks source link

add bound check #168

Closed zhou13 closed 7 years ago

zhou13 commented 7 years ago
#include <iostream>
#include "xtensor/xtensor.hpp"
#include "xtensor/xindexview.hpp"
#include "xtensor/xview.hpp"
#include "xtensor/xio.hpp"
#include <vector>

int main()
{
    xt::xtensor<double, 2> a;
    a(1, 1) = 10;
}

Obviously, I forget to reshape the array before usage. I compile the above code use g++ main.cpp -fsanitize=address -I.. This code runs fine. No runtime error. It is also "valgrind-clean".

I wish:

  1. Most this kind of errors can be detected by -fsanitize=address and valgrind;
  2. The bound check should be enabled by default and I can add something like g++ -DXTENSOR_NDEBUG main.cpp to disable it.
JohanMabille commented 7 years ago

A bound check was added in #207. You can enable it with cmake -DXTENSOR_ENABLE_ASSERT=ON or g++ -DXTENSOR_ENABLE_ASSERT main.cpp.

It is disabled by default.

SylvainCorlay commented 7 years ago

@zhou13 note that this bound check really degrades the performances, so it should only be used for debugging.

zhou13 commented 7 years ago

Generally, bound check will not degrade performance. From the modern computer architecture perspective, boundary check can be done in parallel when doing the memory fetching, which is almost free. In additional, the boundary check is only utilizing the interger ALU, while most applications for scientific computing are either memory bounded or float-point operation bounded.

Some not so rigorous real-world benchmarks show that the performance hit of bound check is usually in 1% magnitude: https://blogs.msdn.microsoft.com/ricom/2006/07/12/cost-of-array-bounds-checking-one-experiment/ https://www.ncbi.nlm.nih.gov/pmc/articles/PMC4487316/

SylvainCorlay commented 7 years ago

In the N-D setting it complicates a bit the picture, so I expect more than 1% hit.

SylvainCorlay commented 7 years ago

So this has been fixed in #207