Gael Guennebaud
2018-05-22 15:31:54 UTC
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
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,
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.
this, I will wait for a confirmation that a patch would be welcome.
Joseph
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.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.
Most critically, there is no workaround for matrix decompositions, for
whichthere 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.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
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/diffhttps://bitbucket.org/eigen/eigen/pull-requests/125/789-
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 onthis, I will wait for a confirmation that a patch would be welcome.
Joseph