Improve wrappers, add handles and NVector #67

alyst · 2016-07-20T14:30:54Z

This is a "loose" rebase of #29 to the master. So it has the same goal (avoid memory leaks by enabling some automatic memory management on the Julia side) but does it a little bit differently.

NVector code (the N_Vector wrapper that manages the memory) is mostly taken from WIP: Add types to facilitate release of memory via finalizers. #29
Handle concept (CVODEMem, IDAMem, KINMem wrapper that calls appropriate XXXFree() methods upon finalization) generalizes similar idea from WIP: Add types to facilitate release of memory via finalizers. #29 by using type parameters
Wrappers generator is enhanced to facilitate Handle and NVector and provide more Julia-friendly methods
- untyped pointers to Sundials objects are replaced with the typed pointers
- in addition to the low-level wrappers that accept pointers and N_Vector, the generator automatically adds higher-levels wrappers that provide automatic conversion of Julia objects (Handle, NVector, Vector) into appropriate low-level types
- user_data is of type Any to facilitate passing Julia objects directly
- definitions from the system headers and implementation details (from xxx_impl.h headers) are not wrapped
- only "sequantial" API for N_Vector is wrapped (parallel one is not)
hand-written higher-level wrappers removed
"simple" API is updated to the new low-level API (+ added support for passing Julia user data to Julia functions) and moved to a separate file

With these changes I was able to fix the N_Vector memory leaks in my Sundials-using code.
But there must be a lot of issues with the current state of the PR (e.g. lifetime of NVectors that are implicitly generated by "higher-level" wrapper functions), so any comments and suggestions are welcome.

cc @acroy @stevengj

acroy · 2016-07-20T15:08:50Z

Thanks! This sounds like a big step forward. I can't have a look at it
before next week though.

On Wed, 20 Jul 2016 at 16:30, Alexey Stukalov [email protected]
wrote:

This is a "loose" rebase of #29
#29 to the master. So it
has the same goal (avoid memory leaks by enabling some automatic memory
management on the Julia side) but does it a little bit differently.

NVector code (the N_Vector wrapper that manages the memory) is
mostly taken from WIP: Add types to facilitate release of memory via finalizers. #29
WIP: Add types to facilitate release of memory via finalizers. #29

Handle concept (CVODEMem, IDAMem, KINMem wrapper that calls
appropriate XXXFree() methods upon finalization) generalizes similar
idea from WIP: Add types to facilitate release of memory via finalizers. #29 WIP: Add types to facilitate release of memory via finalizers. #29 by
using type parameters

Wrappers generator is enhanced to facilitate Handle and NVector and
provide more Julia-friendly methods

untyped pointers to Sundials objects are replaced with the typed
pointers

in addition to the low-level wrappers that accept pointers and
N_Vector, the generator automatically adds higher-levels wrappers
that provide automatic conversion of Julia objects (Handle, NVector,
Vector) into appropriate low-level types

user_data is of type Any to facilitate passing Julia objects
directly

"simple" API is updated to the new low-level API (+ added support
for passing Julia user data to Julia functions) and moved to a separate file

With these changes I was able to fix the N_Vector memory leaks in my
Sundials-using code.
But there must be a lot of issues with the current state of the PR (e.g.
lifetime of NVectors that are implicitly generated by "higher-level"
wrapper functions), so any comments and suggestions are welcome.

cc @acroy https://github.com/acroy @stevengj

https://github.com/stevengj

You can view, comment on, or merge this pull request online at:

#67
Commit Summary

wrap gen: use readdir() instead of pipeing ls

wrap gen: generate typed pointers to Sundials objs

wrap gen: make user_data vars of Any type

wrap gen: allow typeify to return vector of expressions

wrap_gen: add wrappers supporting Julia objects

File Changes

M src/Sundials.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-0 (360)

D src/constants.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-1 (266)

M src/cvode.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-2 (912)

M src/cvodes.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-3 (1669)

A src/handle.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-4 (58)

M src/ida.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-5 (938)

M src/idas.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-6 (1696)

M src/kinsol.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-7 (684)

D src/libsundials.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-8 (259)

M src/nvector.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-9 (235)

A src/nvector_wrapper.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-10 (66)

A src/simple.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-11 (182)

A src/sundials.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-12 (866)

D src/sundials_h.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-13
(2358)

A src/types_and_consts.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-14 (574)

M src/wrap_sundials.jl
https://github.com/JuliaLang/Sundials.jl/pull/67/files#diff-15 (149)

Patch Links:

https://github.com/JuliaLang/Sundials.jl/pull/67.patch

https://github.com/JuliaLang/Sundials.jl/pull/67.diff

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#67, or mute the thread
https://github.com/notifications/unsubscribe-auth/AF4oncO0nb4591ZqnODEfMy6fzaZco4Dks5qXjEggaJpZM4JQ1oi
.

acroy · 2016-07-26T11:46:57Z

@alyst : Thanks again for pushing this! The CIs fail because it seems that you have removed N_Vector, which is still needed? I think it is good that you polished the wrapper generator even if this makes the code review a bit harder. I am not sure though, if the underscored function names are the way to go at this point. I don't know how many people really rely on it, but this would break the code of people who just use the "bare wrapped functions". Maybe we can find another way to have both versions?

alyst · 2016-07-26T12:40:40Z

@acroy I don't know actually why CI complains, because autogenerated N_Vector definitions are still in types_and_consts.jl.
I can remove underscores. I put it mainly for debugging/safety, because if both higher-level wrapper (which does not specify its argument types) and the lower-level function have the same name AND if there is some problem with calling lower-level function from the wrapper (actual argument types do not match the declaration), then the higher-level wrapper would call itself resulting in recursion and non-informative stack overflow error. If the low-level function has different name, then we have a proper diagnostic on the mismatching argument types.
OTOH, since the higher-level wrappers do not put any constraints on their argument types and pointer-to-pointer conversion returns the pointer itself, any old code that uses low-level API would just work.

acroy · 2016-07-26T12:55:06Z

The CI error is indeed strange. Does it run locally for you?

I guess you could check if the old code runs as it is by trying the examples? How about putting the underscored functions into a (sub)module?

alyst · 2016-07-26T13:50:31Z

@acroy Pkg.test("Sundials") runs for me locally (I'm on Julia 0.4.6). And I think I've committed all the changes I've made (except custom paths in wrap_sundials.jl).

I can put low-level functions to a submodule (then the underscores would not be necessary), but the downside is that then you would have to look at two different places in case there's any problem.

acroy · 2016-07-27T07:57:56Z

I tried it locally and get the same error. The problem is that you renamed libsundials.jl to sundials.jl and this somehow leads to Sundials.jl being removed by git/or you removed it accidentally (at least this is what I see here). Maybe you can just keep libsundials.jl.

I can put low-level functions to a submodule (then the underscores would not be necessary), but the downside is that then you would have to look at two different places in case there's any problem.

True, but I think it would be cleaner. If the old code is not broken I don't have a strong preference though. Maybe we should check if there is some sort of convention ...

coveralls · 2016-07-27T08:42:11Z

Coverage decreased (-4.6%) to 5.477% when pulling 35c4744 on alyst:handles into a68a1e8 on JuliaLang:master.

coveralls · 2016-07-27T11:00:23Z

Coverage decreased (-1.7%) to 8.393% when pulling 5d59cbc on alyst:handles into a68a1e8 on JuliaLang:master.

coveralls · 2016-07-27T11:56:47Z

Coverage decreased (-1.8%) to 8.349% when pulling 3d09814 on alyst:handles into a68a1e8 on JuliaLang:master.

alyst · 2016-07-27T11:58:58Z

@acroy Thanks, indeed sundials.jl causes problem on Windows file system and I hadn't understood that libsundials was a workaround. Fixed it.

True, but I think it would be cleaner. If the old code is not broken I don't have a strong preference though. Maybe we should check if there is some sort of convention ...

I'm not sure it would be cleaner. There would be issues with type definitions (N_Vector, CV_SUCCESS etc) -- you would have to decide whether to put it into LowLevel module or to the top module and implement this logic in the wrapper generator. If you are just concerned about "__" prefix, I can remove it, it just makes it easier to detect problems.

acroy · 2016-07-27T12:05:22Z

Great. Let's see what Travis CI has to say (the AppVeyor failure seems unrelated). Have you tried if the examples are still working?

coveralls · 2016-07-27T12:09:02Z

Coverage decreased (-1.8%) to 8.349% when pulling 3d09814 on alyst:handles into a68a1e8 on JuliaLang:master.

coveralls · 2016-07-27T12:13:30Z

Coverage decreased (-1.8%) to 8.349% when pulling 3d09814 on alyst:handles into a68a1e8 on JuliaLang:master.

coveralls · 2016-07-27T12:16:11Z

Coverage decreased (-1.8%) to 8.349% when pulling 3d09814 on alyst:handles into a68a1e8 on JuliaLang:master.

alyst · 2016-07-27T13:58:32Z

@acroy I've just added more examples to the Pkg.test(). That required generating wrapper for Julia->C function pointers convertion. Not all Sundial functional types are supported yet. Also these wrappers require that Julia functions accepts low-level args. But I think improving this is not in the scope of this PR.

coveralls · 2016-07-27T14:03:55Z

Coverage decreased (-0.1%) to 10.019% when pulling c4c51e0 on alyst:handles into a68a1e8 on JuliaLang:master.

coveralls · 2016-07-27T16:59:13Z

Coverage decreased (-0.1%) to 10.019% when pulling ed229cc on alyst:handles into a68a1e8 on JuliaLang:master.

acroy · 2016-07-28T08:42:34Z

@alyst : Thanks! I agree that sorting out all the wrapper issues is beyond the scope of this PR (maybe you could open an issue once this is merged). The important thing is that the example code works. I am a bit puzzled by the failure of the CIs - although it seems to be unrelated?

alyst · 2016-08-16T09:13:47Z

Bump. I've just fixed a few 0.5 issues (the same way that #72 does it), so now all CI configurations are green.

acroy · 2016-08-22T08:59:08Z

Could you rebase once more?

If CIs pass I am +1 for merging, but maybe someone else could have a look as well as the changes are quite substantial.

- update wrapper generator script to replace Ptr{Void} with the typed pointer (e.g. CVODEMemPtr) in the cfunction declaration - correct constants (e.g. CV_SUCCESS) type (should be Cint) - the result of the wrapper script doesn't need further manual editing - rename sundials_h.jl to types_and_consts.jl, remove constants.jl - don't wrap _impl.h headers

alyst · 2016-08-22T11:34:58Z

Rebased. The tests pass except for coverage noise.

I have to say, I will give up rebasing it soon.
I'm eager to take any feedback and implement any changes, but the PR is out there for a month and for the last 4 weeks there were only minor tweaks.

But when I just have rebased today after being asked by @acroy , I have found that I have to do it again, because of the c4f46b2 appearing out of the blue.

I have introduced substantial changes taking into account that recently the rate of commits was rather low. I'm afraid I would not be able to keep up with the master, if that would change.

creates NVector that doesn't own N_Vector

this should prevent N_Vector being destroyed before/during Sundials call also auto-convert Cint arguments

alyst · 2016-08-28T01:18:19Z

It could be that the segfault was triggered by GC running immediately before the low-level Sundials call or during the call (in a user function). At that time the intermediate NVector argument wrapper objects were no longer referenced, so GC removed them destroying the wrapped N_Vectors. I've tried to fix that by saving NVector in a local variable of the high-level wrapper. That guarantees N_Vector is not destroyed during low-level call.

At least Travis doesn't fail for 2 times in a row.

acroy · 2016-08-29T06:03:41Z

@alyst : Great job! Everything seems to work now. Looking at the examples I realized that they need some cleanup, but this should be done separately.

I would suggest that someone tags the current master, merges this and we see how it goes.

* don't use nvector() as it's dangerous * use convert(Vector, ) * use @checkflag * transpose result matrix for memory efficiency * misc code cleanups

* add comments and status messages * don't cd() to the directory where the headers are generated * don't require JULIAHOME env variable

ChrisRackauckas · 2016-08-30T04:49:57Z

I agree with @acroy but would wait for a seal of approval from @tkelman.

tkelman · 2016-08-31T11:56:26Z

I don't really have the bandwidth to review all of this. It looks reasonable at a glance, but is definitely worth at least a new minor version on the first tag that includes it.

alyst · 2016-08-31T13:07:23Z

So to move on the current master should be tagged (as it contains 0.5 compatibility fixes). Then this PR gets in (fingers crossed). Then the new minor version is tagged.

tkelman · 2016-08-31T13:27:05Z

I would tag e3cbb09 rather than master, since that last commit is a bit of a change.

ChrisRackauckas · 2016-09-01T21:23:55Z

I took a look through it and nothing stood out. I was already using a fork of this for awhile too and it works locally. It seems it works locally so I'm merging this. I tagged and put in a PR for e3cbb09 as suggested, and will stage a PR for the agreed upon parts of #75.

ChrisRackauckas · 2016-09-01T21:25:33Z

Given the previous comments by @alyst and @acroy I won't be squash the commits.

lstagner · 2016-09-22T02:06:41Z

I would be a good idea to tag a new version on METADATA. I just got bit by the memory leak this PR fixed.

ChrisRackauckas · 2016-09-22T02:08:25Z

We want to do #75 first to clean up the API, and I got caught up for a little bit.

alyst force-pushed the handles branch from 42045b7 to 35c4744 Compare July 27, 2016 08:36

alyst force-pushed the handles branch from 8b5127e to 3d09814 Compare July 27, 2016 11:36

alyst force-pushed the handles branch from ed229cc to 04f6cec Compare August 13, 2016 23:00

alyst force-pushed the handles branch from 33ae6c4 to a31e61d Compare August 22, 2016 09:52

alyst added 4 commits August 22, 2016 11:55

wrap gen: use readdir() instead of pipeing ls

cd53580

wrap gen: make user_data vars of Any type

ba27040

wrap gen: allow typeify to return vector of expressions

8f163a1

alyst force-pushed the handles branch from a31e61d to 776677a Compare August 22, 2016 11:19

alyst force-pushed the handles branch 2 times, most recently from 644d698 to 00e5979 Compare August 27, 2016 22:20

alyst added 3 commits August 28, 2016 02:47

convert(NVector, N_Vector)

4bbb556

creates NVector that doesn't own N_Vector

wrap gen: create local NVector var in hilevel wrapper

a489e89

this should prevent N_Vector being destroyed before/during Sundials call also auto-convert Cint arguments

wrap gen: normalize include path

f7b6bec

alyst force-pushed the handles branch from 7bb61e7 to 04eb9d3 Compare August 28, 2016 09:50

alyst force-pushed the handles branch from cef09a9 to 47743b7 Compare August 29, 2016 15:03

alyst added 2 commits August 29, 2016 17:12

cleanup examples

b277395

* don't use nvector() as it's dangerous * use convert(Vector, ) * use @checkflag * transpose result matrix for memory efficiency * misc code cleanups

cleanup wrapper generator

a65a152

* add comments and status messages * don't cd() to the directory where the headers are generated * don't require JULIAHOME env variable

alyst force-pushed the handles branch from 47743b7 to a65a152 Compare August 29, 2016 15:13

alyst mentioned this pull request Aug 31, 2016

Adaptive timestepping in high-level interface? #75

Closed

ChrisRackauckas mentioned this pull request Sep 1, 2016

Tag Sundials v0.2.2 [https://github.com/JuliaDiffEq/Sundials.jl] JuliaLang/METADATA.jl#6217

Merged

ChrisRackauckas merged commit 4b8871c into SciML:master Sep 1, 2016

This was referenced Sep 1, 2016

WIP: Add types to facilitate release of memory via finalizers. #29

Closed

Memory leak in cvode() #58

Closed

Memory leaks in N_Vector and cvode #24

Closed

alyst mentioned this pull request Sep 2, 2016

Update Sundials version #77

Closed

ChrisRackauckas mentioned this pull request Sep 2, 2016

Move function wrappers into simplified interface #60

Closed

alyst mentioned this pull request Dec 12, 2016

Add finalizers to mem types #101

Closed

ChrisRackauckas mentioned this pull request Jul 27, 2018

New function Base.unsafe_wrap! to avoid unnecessary heap allocations JuliaLang/julia#28305

Closed

Uh oh!

Improve wrappers, add handles and NVector #67

Improve wrappers, add handles and NVector #67

Uh oh!

Conversation

alyst commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acroy commented Jul 20, 2016

https://github.com/stevengj

Uh oh!

acroy commented Jul 26, 2016

Uh oh!

alyst commented Jul 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acroy commented Jul 26, 2016

Uh oh!

alyst commented Jul 26, 2016

Uh oh!

acroy commented Jul 27, 2016

Uh oh!

coveralls commented Jul 27, 2016

Uh oh!

coveralls commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyst commented Jul 27, 2016

Uh oh!

acroy commented Jul 27, 2016

Uh oh!

coveralls commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 27, 2016

Uh oh!

coveralls commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyst commented Jul 27, 2016

Uh oh!

coveralls commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acroy commented Jul 28, 2016

Uh oh!

alyst commented Aug 16, 2016

Uh oh!

acroy commented Aug 22, 2016

Uh oh!

alyst commented Aug 22, 2016

Uh oh!

alyst commented Aug 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acroy commented Aug 29, 2016

Uh oh!

ChrisRackauckas commented Aug 30, 2016

Uh oh!

tkelman commented Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyst commented Aug 31, 2016

Uh oh!

tkelman commented Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisRackauckas commented Sep 1, 2016

Uh oh!

ChrisRackauckas commented Sep 1, 2016

Uh oh!

lstagner commented Sep 22, 2016

Uh oh!

ChrisRackauckas commented Sep 22, 2016

alyst commented Jul 20, 2016 •

edited

Loading

alyst commented Jul 26, 2016 •

edited

Loading

coveralls commented Jul 27, 2016 •

edited

Loading

coveralls commented Jul 27, 2016 •

edited

Loading

coveralls commented Jul 27, 2016 •

edited

Loading

coveralls commented Jul 27, 2016 •

edited

Loading

coveralls commented Jul 27, 2016 •

edited

Loading

coveralls commented Jul 27, 2016 •

edited

Loading

alyst commented Aug 28, 2016 •

edited

Loading

tkelman commented Aug 31, 2016 •

edited

Loading

tkelman commented Aug 31, 2016 •

edited

Loading