Discussion:
[eigen] Maximum size at compile time
Gael Guennebaud
2018-05-22 15:31:54 UTC
Permalink
Hi,

if someone is willing to finalize and cleanup this PR, then we're still ok
to merge it. I quickly looked at the PR and I think lot of boirlerplate
codes and redundancies could be avoided by delegating the handling of the
reserved memory to a base class of DenseStorage instead of messing with
plain_array. It also remains to:
- make sure that this option is propagated to decompositions,
- add documentation
- (optional) add a PlainObjectBase::reserve(Index size) method enabled only
if Options has the proper flag.

gael
Hi Adrien,
Hi Joseph,
we have the same issue in our lab, and each time I start a new project I
kick myself for not pushing this feature during the Eigen3 beta.
The use of Map is not really satisfactory and error prone.
I did not think about it.
Most critically, there is no workaround for matrix decompositions, for
which
there will be reallocation whenever the size of the matrix changes.
Basically, there would be a need of a memory management mode in which a
reallocation occurs only if the new buffer needs to be bigger than the
current one. For now, the size of the buffer is supposed to be
row()*cols(), so we would need an additional integer to keep track of
the buffer size separately from the matrix size, which would break the
ABI.
My intent is to add a flag to the Option template argument of Matrix.
This will not break ABI compatibility since it will be two different C++
types.
I did not check carefully matrix decomposition classes. I expect them to
take as template parameter a matrix type which would then contain the
memory management flag.
The only issue I see with this method is the non-compatibility between
Matrix<EnableMaxSizeAtRunTime> and Matrix<! EnableMaxSizeAtRunTime>.
Maybe it is possible to play with cast operator to fix this issue.
Joris Vaillant began to tackle the problem a few years ago, but the PR
https://bitbucket.org/eigen/eigen/pull-requests/125/789-
memory-allocation-only-when-necessary/diff
Maybe we could try to put it back on rail.
Thanks for the link. I'll have a look. Before I put too much effort on
this, I will wait for a confirmation that a patch would be welcome.
Joseph
Loading...