Leak of constraint functions

[Yen]

Hello.

Apologies if this is incorrect but we are assessing different solver libraries as the one we are currently using has issues with leaking boxed callbacks. It seems this is handled within the optimize function for the primary callback with fn_cfg_ptr being converted back into a Box and then dropped, but the same is not done for the constraint allocations c_cfg_ptr.

I am also wondering if the boxing of values here, as there are several more done within the optimize function, are unnecessary. This boxing is done and then references are passed to the converted C code, but seeing as none of the pointers passed outlive the optimize function, I think the boxing is unnecessary and passing in a pointer to the stack variables would be valid?

I have not looked into the generated Rust from C code, so I am not sure if this is a misunderstanding of how the C takes ownership of allocations.

Thanks

[relf]

Thanks for the heads up! Indeed, seems constraint allocations are not dropped. The drop of fn_cfg_ptr was actually a previous fix (see #1), I guess I missed to check for constraint functions. Do you want to submit a PR?

Regarding your second remark, As we allocate NLoptFunctionCfg and NLoptConstraintCfg structures on the heap we need to clean them up at the end otherwise we have memory leaks.

[Yen]

Regarding your second remark, As we allocate NLoptFunctionCfg and NLoptConstraintCfg structures on the heap we need to clean them up at the end otherwise we have memory leaks.

Understood, but my question was more as to why they are allocated on the heap in the first place? The value does not need to outlive the scope so passing a reference to the value on the stack would be the same and avoid the allocation/cleanup?

let fn_cfg = NLoptFunctionCfg {
    objective_fn: func,
    user_data: args.clone(),
};
let fn_cfg_ptr = &fn_cfg as *const _ as *mut c_void;

Same logic can be applied to the constraints with a pair of vecs instead.

There are a whole lot of Box::from_raw related things in the generated code, but its a bit too much to go into for now. If it is a result of a conversion though I think its safe to assume that's all correct and the mistakes are only in the minimize function itself.

[relf]

Ok, actually I think I just mimicked the code I saw around to make it work and was not aware of the conversion from a Rust reference to a raw C pointer. Thanks!

[relf]

Fixed by #27

[Yen]

Thanks for the merge! We are not using this lib at the moment but might try it out soon. If we do end up using it, I will likely go over the generated code and optimise/remove allocations similar to this where possible. I know this is a conversion from the C code but are you open to significant refactors within the generated code?

Our problem space is finance so performance is a key thing for us, and with this being MIT, we can ofc maintain our own fork for internal use, but I can also submit patches if you would want to merge in performance and not feature-related changes.

[relf]

Well... I would be interested by performance improvements but on the other side, as you might guess, the idea was to be able to regenerate code from rust-nlopt (currently nlopt 2.9.1) which follows upstream nlopt (currently 2.10.1) while cobyla is atm from nlopt 2.7.1 😅 .
I've just seen there are still changes between 2.7.1 and 2.10.1 on cobyla C code! I will try to update at least to rust-nlopt 2.9.1.
So I am curious to see the kind of performance improvement you can get, but not sure I want to loose the ability to regenerate atm.

[Yen]

Yep, that was my question really. Understandable if you want to be able to update, I won't worry about it here then as it would change quite a lot and the upgrade path would not be nice.

(For reference, rust-nlopt is the library we have issues with at the moment, it suffers from similar problems but worse in that it's not an easy fix like this.)

[relf]

FYI, I've just regenerated the code (see 1.0.2-rc.1). Finally almost no change in the original C code.

Actually, the changes were located in the wrapping code which fixes a cobyla issue (see #29), so this issue is not fixed in the cobyla Rust code.