Smart pointers in interface?

[marku...@esat.kuleuven.be]

I have a suggestion for a possible future enhancement. In current
versions of ceres, you pass raw pointers to - say - AddResidualBlock,
and ceres then takes over ownership. This is potentially problematic
as it is hard to make that exception-safe. A possible solution is to
change the interface to accept some kind of smart pointer. If the
internal workings are not to be changed, then something similar to an
auto_ptr (or a C++11 shared_ptr) should work nicely, i.e. a pointer
with an ownership flag. When passed to a function that takes
ownership, the flag would be cleared, but the pointer would be left
untouched. That way, one could write something like

ceres_ptr<ceres::CostFunction> cost_function(new SomeCostFunction());
ceres_ptr<ceres::LossFunction> loss_function(new SomeLossFunction());
problem.AddResidualBlock( cost_function, loss_function, params );

in an exception-safe manner.

[marku...@esat.kuleuven.be]

On Jun 11, 8:09 pm, "markus.m...@esat.kuleuven.be"

<markus.m...@esat.kuleuven.be> wrote:
> something similar to an auto_ptr (or a C++11 shared_ptr) should work nicely,

I meant "(or a C++11 unique_ptr)"

[Sameer Agarwal]

Hi Markus,

I do not have any experience with exception safety in c++. Keir is the expert there, but I would like to understand what kind of error are you trying to avoid with the use of smart pointers here and what kind of exception safety are you looking for?

Ceres internally does not use any exceptions and we have no plans for supporting exceptions either right now.

Sameer

--
----------------------------------------
Ceres Solver Google Group
http://groups.google.com/group/ceres-solver?hl=en?hl=en

[Keir Mierle]

Hi Markus,

Sameer is correct in that Ceres is not coded in an exception safe way, since we do not use exceptions at Google. With that said, Ceres itself is unlikely to throw any exceptions other than memory allocation exceptions (due to insufficient memory). Personally, I do not object to making Ceres exception safe provided the changes are not overly invasive. I think it's in realm of possibility; however, we are unlikely to do it ourselves.

Looking at your example, I don't see a cause for concern. Is your worry that the call to AddResidualBlock() will throw? As long as the problem destructor runs, then the cost/loss functions will get freed.

Although we do not officially support this, Ceres has a "unique_ptr" equivalent in "include/ceres/internal/scoped_ptr.h". Here is how we would code what you suggest at Google:

scoped_ptr<ceres::CostFunction> cost_function(new SomeCostFunction());

scoped_ptr<ceres::LossFunction> loss_function(new SomeLossFunction());

// ... other code which might throw, but will have the cost/loss cleaned up due to the scoped_ptr

problem.AddResidualBlock( cost_function.release(), loss_function.release(), params );

The ".release()" method returns the current pointer contained of the scoped_ptr, then zeros out the internal pointer, transferring ownership to whomever recieves the ".release()" expression.

If you plan to use scoped_ptr, please copy it directly into your project, since it is not part of the supported public Ceres API.

Hope that helps,

Keir

[Markus Moll]

Hi

> I do not have any experience with exception safety in c++. Keir is the
> expert there, but I would like to understand what kind of error are you
> trying to avoid with the use of smart pointers here and what kind of
> exception safety are you looking for?

I was thinking of the following:

problem.AddResidualBlock(
new MyCostFunction(cost_params),
new MyLossFunction(loss_params),
getParameters());

If either getParameters or one of the constructors involved here throws an
exception, there is a potential memory leak (if the cost function or the loss
function object has already been created). You can work around this, but it
can quickly get tedious.
Now with a properly designed smart pointer, the code would look like:

smart_ptr<CostFunction> cost_function(new MyCostFunction(cost_params));
smart_ptr<LossFunction> loss_function(new MyLossFunction(cost_params));
problem.AddResidualBlock(cost_function, loss_function, getParameters());

Here, in case of an exception, all memory will be properly freed when
unwinding the stack.

At the moment, the user must take care that no exceptions are thrown, which
may or may not be possible. Exceptions might be raised by e.g. logging
facilities, parameter checks, other libraries or even by new itself
(bad_alloc). Admittedly this will happen only in exceptional (no pun intended)
circumstances, but the necessary changes are relatively small (I think).

Markus

[Markus Moll]

Hi

> Looking at your example, I don't see a cause for concern. Is your worry that
the call to AddResidualBlock() will throw? As long as the problem destructor
runs, then the cost/loss functions will get freed.

No, the concern is more that I have to make sure that e.g. the cost function
constructor cannot check parameters (and throw if they are invalid). It's not
a biiiig issue.

> Although we do not officially support this, Ceres has a "unique_ptr"
equivalent in "include/ceres/internal/scoped_ptr.h". Here is how we would code
what you suggest at Google:
>
>
> scoped_ptr<ceres::CostFunction> cost_function(new SomeCostFunction());
> scoped_ptr<ceres::LossFunction> loss_function(new SomeLossFunction());
> // ... other code which might throw, but will have the cost/loss cleaned up
due to the scoped_ptr
> problem.AddResidualBlock( cost_function.release(), loss_function.release(),
params );

That's the obvious solution, but it fails as soon as you want to use the same
loss (or cost) function object more than once. (At which point you'd have to
keep another dumb pointer... it will work, but it's arguably ugly:

scoped_ptr<ceres::LossFunction> loss_function_sp(new SomeLossFunction());
ceres::LossFunction *loss_function = 0;
// do something
while(some_condition) {
// do something else
if(!loss_function) loss_function = loss_function_sp.release();
problem.AddResidual( loss_function, cost_function, params );
}

)

I'll see if I can implement a simple solution.

Markus

[Keir Mierle]

Hi Markus,

Be warned that I'm (very) tepid on including any sort of smart pointer in the supported-forever-public-API. More comments below.

If this is such a burden, note that you can tell Ceres's Problem object to not own either cost functions or loss functions (independently). In this case, it looks like a simpler solution would be to stack allocate the loss function and tell Ceres to not own it.

Keir

 

Markus

[Markus Moll]

Hi

> On Monday 11 June 2012 13:06:39 Keir Mierle wrote:
>
> Be warned that I'm (very) tepid on including any sort of smart pointer in
> the supported-forever-public-API. More comments below.

Thanks for the warning, in this case I'll save some time and won't try.

> > scoped_ptr<ceres::LossFunction> loss_function_sp(new SomeLossFunction());
> > ceres::LossFunction *loss_function = 0;
> > // do something
> > while(some_condition) {
> > // do something else
> > if(!loss_function) loss_function = loss_function_sp.release();
> > problem.AddResidual( loss_function, cost_function, params );
> > }
> >
> > )
> >
>

> If this is such a burden,

No sarcasm please ;-)
Of course it's not a heavy burden, I just wanted to motivate and suggest a
different and in my opinion cleaner solution. If that doesn't fit well, no
problem.

> note that you can tell Ceres's Problem object to not own either cost
> functions or loss functions (independently). In this case, it looks like a
> simpler solution would be to stack allocate the loss function and tell Ceres
> to not own it.

Except that I'm collecting various terms from various sources, so this is
adding the residual block within a function different from where the solver is
called. But now that's very specific to my code...

Thanks
Markus

[Keir Mierle]

On Mon, Jun 11, 2012 at 1:21 PM, Markus Moll <marku...@esat.kuleuven.be> wrote:

Hi

> On Monday 11 June 2012 13:06:39 Keir Mierle wrote:
>
> Be warned that I'm (very) tepid on including any sort of smart pointer in
> the supported-forever-public-API. More comments below.

Thanks for the warning, in this case I'll save some time and won't try.

> > scoped_ptr<ceres::LossFunction> loss_function_sp(new SomeLossFunction());
> > ceres::LossFunction *loss_function = 0;
> > // do something
> > while(some_condition) {
> >  // do something else
> >  if(!loss_function) loss_function = loss_function_sp.release();
> >  problem.AddResidual( loss_function, cost_function, params );
> > }
> >
> > )
> >
>

> If this is such a burden,

No sarcasm please ;-)
Of course it's not a heavy burden, I just wanted to motivate and suggest a
different and in my opinion cleaner solution. If that doesn't fit well, no
problem.

I promise no snark was intended! This is what I get for writing email too quickly. My team is about to go into code freeze so pressure is high.

We appreciate a different perspective, and want to address your concerns while balancing a conservative approach to API evolution.

 

> note that you can tell Ceres's Problem object to not own either cost
> functions or loss functions (independently).  In this case, it looks like a
> simpler solution would be to stack allocate the loss function and tell Ceres
> to not own it.

Except that I'm collecting various terms from various sources, so this is
adding the residual block within a function different from where the solver is
called. But now that's very specific to my code...

I will think about this some more. We could get our own unique_ptr that has an implicit constructor; that way, we don't need extra function signatures and existing code would continue to work. Sameer, any thoughts?

Another possibility is to provide a contrib/ API that uses smart pointers but is a layer on top of our API instead of part of the core API. After having some field time, we can reconsider if such an API is suitable for the core.

Keir

 

Thanks

[Sameer Agarwal]

I will think about this some more. We could get our own unique_ptr that has an implicit constructor; that way, we don't need extra function signatures and existing code would continue to work. Sameer, any thoughts?

I am very tepid on expanding the core API, especially since the state of smart pointers is stil evolving from what I can tell. 

Another possibility is to provide a contrib/ API that uses smart pointers but is a layer on top of our API instead of part of the core API. After having some field time, we can reconsider if such an API is suitable for the core.

this basically means that the api now lives in two files instead of one. This requires some careful thought.

Sameer