deephealthproject / eddl

European Distributed Deep Learning (EDDL) library. A general-purpose library initially developed to cover deep learning needs in healthcare use cases within the DeepHealth project.
https://deephealthproject.github.io/eddl/
MIT License
34 stars 10 forks source link

Tensor size is limited to 4 GiB #264

Closed CostantinoGrana closed 3 years ago

CostantinoGrana commented 3 years ago

In the definition of Tensor, the size is defined as https://github.com/deephealthproject/eddl/blob/c0eeb39a6456d656d5315dee44eb90ee2715fb75/include/eddl/tensor/tensor.h#L93-L94 long is not making int any longer in any system we recompiled EDDL. Will we ever need a layer with data larger than 4 GiB? I don't think so, but in any case why having that long?

salvacarrion commented 3 years ago

It is a sanity check.

As far as I know, the ranges of those types are implementation dependant. For instance, unsigned int is guaranteed to be able to represent values from 0 to 65535, while unsigned long int is guaranteed to be able to represent values in the range 0 to 4294967295.

However, beyond those guarantees, they can collapse into the same range as is the case with Visual Studio. Both, unsigned int and unsigned long int end up having the same bit-range representation (either 32 or 64 bits depending of the target platform).

jonandergomez commented 3 years ago

I suggest to use uint64_t as the data type for attributes/variables representing the size of anything. It is C++ standard and will be available in any C++ compiler and operating system. The alternative is size_t, that is also uint64_t since a version earlier than C++ 11, I guess, not sure now.

CostantinoGrana commented 3 years ago

I know that in theory int and long int are different. I was just pointing out that in all compilers we and you have used (gcc, msvc, Intel) they are 32 bits. I don't think you really intend to compile EDDL on machines where int is 16 bits...

If we have a reason to require 64 bits, let's do it, otherwise size_t would be reasonable.

salvacarrion commented 3 years ago

I like size_t proposal

jonandergomez commented 3 years ago

sizeof(size_t) = 8 bytes, i.e., 64-bit integer, it is the standard widely used as the data type for attributes/variables representing sizes.

Let use it!

CostantinoGrana commented 3 years ago

TL;DR I agree with the size_t

Sorry, I was wrong and @salvacarrion was right. It's not a compiler thing, but an OS thing. So GCC under Windows has 32 bits long int, while gcc under Linux has 64 bits long int. Tested with:

#include <iostream>
#define PS(x) std::cout << "sizeof(" #x ") = " << sizeof(x) << "\n";
int main() {
    PS(char);
    PS(short int);
    PS(int);
    PS(long int);
}

GCC under Linux:

sizeof(char) = 1
sizeof(short int) = 2
sizeof(int) = 4
sizeof(long int) = 8    <----

GCC under Windows:

sizeof(char) = 1
sizeof(short int) = 2
sizeof(int) = 4
sizeof(long int) = 4    <----

And by reading well here under the "Data models" section its clear that:

LLP64 or 4/4/8 (int and long are 32-bit, pointer is 64-bit) = Win64 API LP64 or 4/8/8 (int is 32-bit, long and pointer are 64-bit) = Unix and Unix-like systems (Linux, macOS)

So, @salvacarrion, if you used unsigned long int to allow for more than 4 GiB in size, it's a problem under Windows.

@jonandergomez Just to be clear, size_t is 32 bits when compiling at 32 bits, 64 bits when compiling at 64 bits, both on Windows and on Linux.

Closing this long rant, size_t is the way to go to ensure the ability of going beyond 4 GiB in a single Tensor. I still don't know if this will ever be needed!

jonandergomez commented 3 years ago

Wow, I thought size_t was a 64-bit unsigned integer in any operating system. Thanks Costantino for the clarification. I still support to use size_t because it will unlikely we compile the library for 32-bit architectures, and if it is the case, it will not be possible to manage tensors (or any other type of memory blocks) larger than 4GB in 32-bit architectures anyway. So, if possible, let us use size_t.