Plan to move to 64bit indexing

[Александр Иванов]

Problem. The question about usage of `std::int64_t`/`std::uint64_t` indexing raised multiple times https://github.com/ceres-solver/ceres-solver/issues/684 but the problem is that changing `int` to `std::int64_t` is a tedious work and requies very accurate investigation of the all usages of `rows`, `cols`, `num_nonzeros` etc. Actually the main pain is about  `num_nonzeros`. It is very strange to see it overflows in sparse matrices when task is higher than 4GB (num_residuals x num_paramters > 2**32). I have a PC station with 256GB RAM and about 24 cores and it is very strange to see that I cannot use it fully on comparitively small tasks. Currently I solve SLAM like problems with `g2o` or using another solver type, but it seems that we can use.

Solution. It consists of multiple stages but it seems that eventually we can put the most of tedious work onto compiler (I used `clang-14`)

1.  Introducing bazel build with `clang` (to use its multiple compiler options). I prefer `bazel` since it simplifies compiler options control. To tell the truth I failed to disable compiler options for mulitple system and third party libraries using `CMake`. Moreover the project I work on mainly uses `bazel` so I a little bit stick to it.
   
2. Enabling all conversions checking from here (https://clang.llvm.org/docs/DiagnosticsReference.html). First of all we are interested in `-Wshorten-64-to-32` since if we change `num_nonzeros_` to int64_t any operation including this value will raise compiler warning so we will be able to identify all usages
3. Introducing `numeric_cast`. Idea comes from boost::numeric_cast and from this proposal to C++ standard `https://github.com/qingfengxia/cpp_numeric_cast/blob/master/numeric_cast.h`. Actually even if we find all casts it does not prevent `static_cast`s from errors when we perform something like this `std::int32_t a = static_cast<std::int32_t>({expression of type std::int64_t})`. Here we simply demonstrate the compiler that we are familiar about conversion but there are no guarantees about its correctness. I have written simple numeric_cast.h just to check conversions from `int64_t` to `int32_t`. Full accurate numeric_cast is about 250 lines of code and seems to be a very good solution to use everywhere instead of `static_cast<T>(S)`. Also we can control its usage with compiler options. If somebody desires ordnary behaviour it can be achieved with `using numeric_cast = static_cast` under `#ifndef USE_NUMERIC_CAST` in `numeric_cast.h`
4. At this point we will be almost ready to start changing int32 to int64. Just one question shall be answered: are we going to use `int64_t` or `std::size_t` (int32 for 32bit platforms and in64t for 64bit platforms).  One option is to create alias `index_t` to avoid problems and experiment with it. Another options is to use int64_t everywhere (seems more convenient since we anyway will fail to allocate more memory than available on the system)
   

Locally I achieved stage 3 but right now I see that amount of changes grown significantly. Also seeing that this problem is a painful one for many users I want to help to solve it not only in my project but also in public repo. But here is a point to discuss this plan since it would not be good if I start to perform this changes without some approval from maintainers.

[Sameer Agarwal]

This is an interesting proposal. I have a busy week, so let me think about it and send a detailed reply next week.

Sameer

--
You received this message because you are subscribed to the Google Groups "Ceres Solver" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ceres-solver...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/ceres-solver/39c3147e-990c-4726-852d-af6553530883n%40googlegroups.com.

[Sameer Agarwal]

one thing I would mention is that the bazel build is a much more restricted build than the full cmake build. As a result it does not support any of the sparse backends except for eigen, which will require substantial change since the underlying libraries are not templated on the indexing type. So it would be worth making this work with cmake to make sure we do not miss things in the large build.

Sameer

[Александр Иванов]

I agree with you that everything must be checked with both CMake and bazel. Personally I also wish to make bazel build support also sparse backends. Right now bazel build is a little bit strange since it does not use system Eigen with sparse backend but downloads new one with http_archive. It is a simple solution but does not seem good to me. We need to add system Eigen to WORKSPACE file.

среда, 11 января 2023 г. в 17:57:33 UTC+3, sameer...@google.com:

[Sameer Agarwal]

The reason there is no support for other sparse backends is because SuiteSparse did not have support for bazel.

I am happy to accept improvements to tbe bazel build.

Sameer 

To view this discussion on the web visit https://groups.google.com/d/msgid/ceres-solver/b024a838-489c-4d3d-9c98-d3e70807bc2an%40googlegroups.com.

[Sameer Agarwal]

Alexander,

I think your idea for first making the ceres code safe w.r.t `-Wshorten-64-to-32` is a good place to start. 

Once this is done, then we need to make a decision about introducing a compile time choice of an index_t type, and then use it consistently in the code. 

The latter will require some prototyping and planning as to how we plan to land this rather large change to the internals of ceres.

Sameer