Make code safe for large (>2G) buffers - #247
Conversation
…ume no axis is longer than 32 bits
2164db9 to
e17c7d7
Compare
mhasself
left a comment
mhasself
left a comment
There was a problem hiding this comment.
Since I started this, you said you knew it was incomplete. But anyway, here are all the things I could find.
It does look like a lot of these inline casts would be unnecessary if we just did detector index loops as for (int64_t di=0; .... I'm totally ok with that approach.
Generally this is a huge pervasive problem, and it almost seems like the solution is "don't use int anymore, ever, for anything". If I were overhauling this myself I would probably define a type to use for any integers that might get combined into array indices or offsets. C++ has ptrdiff_t -- that would do the trick.
But also -- the parts of the code that are immune to this issue are instructive: if you make use of the BufferWrapper strides and pointer computation, these offset issues don't occur. And as a bonus, you can handle slightly non-trivial data layouts more easily.
It's annoying to need to leave int behind! So easy to type ...
| template <typename T> | ||
| static std::string shape_string(std::vector<T> shape) |
There was a problem hiding this comment.
Any reason not to just change this to <int64_t>?
There was a problem hiding this comment.
If we change shape below to be int_64 then than is fine. I had avoided doing that because we had wanted to proceed with the assumption that the shape of the array should be limited s.t it is always int in each dimension. Hence size stays an int (and thus ndet and nsamp stay int) but vshape is allowed to be int64_t so that we can check it even if it is more than an int.
|
|
||
| // Constructor with shape and type checking. | ||
| BufferWrapper(std::string name, const bp::object &src, bool optional, | ||
| std::vector<int> shape) |
There was a problem hiding this comment.
Constructor can/should expect a vector<int64_t> shape, now, right?
| for(int di = 0; di < ndet; di++) | ||
| for(int i = b1; i < b2; i++) | ||
| ft_[di*nmode+i] *= biD[di]/norm; | ||
| ft_[(int64_t)(di*nmode+i)] *= biD[di]/norm; |
There was a problem hiding this comment.
Not sufficient ... must cast one of the multiplicands directly; e.g.
ft_[(int64_t)di*n_mode+i] ...
Note also that (*ft_buf.ptr_2d(di, i)) would get this same reference.
| for (auto const &r: rangemat[di].segments) | ||
| for(int j = r.first; j < r.second; j++, i++) | ||
| vals[i] = tod[di*nsamp+j]; | ||
| vals[i] = tod[(int64_t)(di*nsamp+j)]; |
| for (auto const &r: rangemat[di].segments) | ||
| for(int j = r.first; j < r.second; j++, i++) | ||
| tod[di*nsamp+j] = vals[i]; | ||
| tod[(int64_t)(di*nsamp+j)] = vals[i]; |
| #pragma omp parallel for num_threads(nthreads) | ||
| for (int i = 0; i < ndets; ++i) { | ||
| int ioff = i * row_stride; | ||
| int64_t ioff = i * row_stride; |
| #pragma omp parallel for num_threads(nthreads) | ||
| for (int i = 0; i < ndets; ++i) { | ||
| int ioff = i * row_stride; | ||
| int64_t ioff = i * row_stride; |
| #pragma omp parallel for num_threads(nthreads) | ||
| for (int i = 0; i < ndets; ++i) { | ||
| int ioff = i * row_stride; | ||
| int64_t ioff = i * row_stride; |
| const int ndets = tod_buf->shape[0]; | ||
| const int nsamps = tod_buf->shape[1]; | ||
|
|
||
| int row_stride = tod_buf->strides[0] / sizeof(T); |
| def test_overflow(self): | ||
| a = np.zeros((4050, 603260), dtype='float32') | ||
| so3g.block_minmax(a, a, 800, 2, 0) | ||
|
|
There was a problem hiding this comment.
Can we add a bunch of unit tests for the "large array limit" of all the array_ops functions? They could be disabled by default but it would be nice to run them, against this PR for example, or anytime we add new stuff.
3 participants
No description provided.