Re: [Rdkit-discuss] c++ atomic lifetime
Jason Biggs On Fri, Aug 28, 2020 at 8:16 PM dmaziuk via Rdkit-discuss < rdkit-discuss@lists.sourceforge.net> wrote: > On 8/27/2020 8:48 PM, Jason Biggs wrote: > > > > I'm not very familiar with how the python interface works, is there a > > similar issue with the python wrappers? Does the wrapper class for the > > Atom clean up after itself differently if the atom is marked as having an > > owner? > > There Be Dragons. > > Python VM does reference counting on its own objects and will destroy > them for you at some point. Exactly how it works out with objects > created by external libraries is an interesting question. > > SWIG, for example, creates a "proxy" python object for each c++ one, > with a flag that tells the runtime to either destroy the underlying c++ > object when the "proxy" is garbage-collected, or not. E.g. if you > garbage-collect an Atom on python side, you have no idea if destroying > its linked c++ Atom will mess up its c++ ROMol container, so on > container/element-type objects the flag's typically a "not". > This is helpful, thank you. If I do end up exposing the Atom in top level (mathematica in my case), then I will need to do something similar with the wrapper class. Jason > > Dima > > > ___ > Rdkit-discuss mailing list > Rdkit-discuss@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/rdkit-discuss > ___ Rdkit-discuss mailing list Rdkit-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
Hi Jason, On Fri, Aug 28, 2020 at 3:50 AM Jason Biggs wrote: > > In my application, I have a wrapper class that I expose to top level > users, which holds a unique pointer to an ROMol. I know then that when my > wrapper class member goes away so does the ROMol. What I don't have is a > similar wrapper class for an Atom, precisely because of these ownership > questions - any atom properties or modifications go through the ROMol > wrapper class. > > I'm not very familiar with how the python interface works, is there a > similar issue with the python wrappers? Does the wrapper class for the > Atom clean up after itself differently if the atom is marked as having an > owner? Hope that question isn't too vague > Lifetime management of pointers, particularly pointers that are connected to each other (like molecules and the atoms that come from them) is, indeed, one of those tricky things. For the RDKit Python wrappers it actually ends up being pretty straightforward because boost::python, the library we use to create the wrappers, provides tooling for this case. Within boost::python when you return a pointer to an object you have to specify who owns the memory (i.e. is it an internal reference or a pointer to new memory that Python should clean up when it's done with it). Accompanying this is another mechanism that allows you to specify whether or not the lifetimes of objects should be linked. For example, when we expose the method Mol::GetAtomWithIdx() ( https://github.com/rdkit/rdkit/blob/master/Code/GraphMol/Wrap/Mol.cpp#L350) we tell boost::python that something else owns the pointer to the atom (in this case it's the molecule) and that the molecule should not be deleted as long as the atom still exists. -greg ___ Rdkit-discuss mailing list Rdkit-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
On 8/27/2020 8:48 PM, Jason Biggs wrote: I'm not very familiar with how the python interface works, is there a similar issue with the python wrappers? Does the wrapper class for the Atom clean up after itself differently if the atom is marked as having an owner? There Be Dragons. Python VM does reference counting on its own objects and will destroy them for you at some point. Exactly how it works out with objects created by external libraries is an interesting question. SWIG, for example, creates a "proxy" python object for each c++ one, with a flag that tells the runtime to either destroy the underlying c++ object when the "proxy" is garbage-collected, or not. E.g. if you garbage-collect an Atom on python side, you have no idea if destroying its linked c++ Atom will mess up its c++ ROMol container, so on container/element-type objects the flag's typically a "not". Dima ___ Rdkit-discuss mailing list Rdkit-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
On 8/28/2020 12:30 AM, Paul Emsley wrote: Isn't this the soft of undefined behaviour that one would expect when accessing deleted memory? Try adding some code between the deletion of mol and the access of atom that allocates and deallocs some memory for a second or so. Anyway, I wouldn't try to "out-clever" the RDKit by deleting molecules "by hand." Like I said before, in principle it's only an issue if your program is running forever and keeps creating new ROMols without ever deleting the old ones: then you have a "memory leak" and will eventually run out of memory. If your code runs to a stop you normally needn't worry about this. One of the caveats is GUI enviroments where the program is "running" as long as its window is around: users who tend to keep bazillion windows open forever may run into OOM problems. Dima ___ Rdkit-discuss mailing list Rdkit-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
Hi Jason.
to pinpoint potential memory issues you may run your code through valgrind.
For example, it would flag access to previously freed memory in your
program:
1 #include
2 #include
3 #include
4
5 int main() {
6 auto mol = RDKit::SmilesToMol("");
7 auto atom = mol->getAtomWithIdx(0);
8 auto m2 = atom->getOwningMol();
9 std::cout << "Z=" << atom->getAtomicNum() << std::endl; //
prints Z=6
10 delete mol;
11 std::cout << "Z=" << atom->getIdx() << std::endl; // prints
Z=0
12 std::cout << "N=" << m2.getNumAtoms() << std::endl;//
prints N=4
13 delete atom; // seg fault
14 }
$ valgrind ./testmem
==11551== Memcheck, a memory error detector
==11551== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==11551== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==11551== Command: ./testmem
==11551==
Z=6
==11551== Invalid read of size 4
==11551==at 0x10C924: RDKit::Atom::getIdx() const (Atom.h:133)
==11551==by 0x10B4B6: main (testmem.cpp:11)
==11551== Address 0x5a85a84 is 52 bytes inside a block of size 72 free'd
==11551==at 0x4C2B51D: operator delete(void*) (vg_replace_malloc.c:586)
==11551==by 0x40A5F83: RDKit::ROMol::destroy() (in
/usr/prog/scicomp/pythonds/develop/conda/lib/libRDKitGraphMol.so.1.2020.03.5)
==11551==by 0x40AC66E: RDKit::RWMol::~RWMol() (in
/usr/prog/scicomp/pythonds/develop/conda/lib/libRDKitGraphMol.so.1.2020.03.5)
==11551==by 0x10B494: main (testmem.cpp:10)
==11551== Block was alloc'd at
==11551==at 0x4C2A593: operator new(unsigned long)
(vg_replace_malloc.c:344)
==11551==by 0x4EA4F6B: yysmiles_lex(YYSTYPE*, void*, int&) (in
/usr/prog/scicomp/pythonds/develop/conda/lib/libRDKitSmilesParse.so.1.2020.03.5)
==11551==by 0x4E99E8F: yysmiles_parse(char const*,
std::vector >*, RDKit::Atom*&,
RDKit::Bond*&, std::__cxx11::list >*, void*, int&) (in
/usr/prog/scicomp/pythonds/develop/conda/lib/libRDKitSmilesParse.so.1.2020.03.5)
==11551==by 0x4E5B192: RDKit::(anonymous
namespace)::smiles_parse_helper(std::__cxx11::basic_string, std::allocator > const&, std::vector >&, RDKit::Atom*&, RDKit::Bond*&,
int) (in
/usr/prog/scicomp/pythonds/develop/conda/lib/libRDKitSmilesParse.so.1.2020.03.5)
==11551==by 0x4E5B5CA: RDKit::(anonymous
namespace)::smiles_parse(std::__cxx11::basic_string, std::allocator > const&,
std::vector >&) (in
/usr/prog/scicomp/pythonds/develop/conda/lib/libRDKitSmilesParse.so.1.2020.03.5)
==11551==by 0x4E5CB24: RDKit::toMol(std::__cxx11::basic_string, std::allocator > const&, int
(*)(std::__cxx11::basic_string, std::allocator > const&, std::vector >&), std::__cxx11::basic_string, std::allocator >
const&) (in
/usr/prog/scicomp/pythonds/develop/conda/lib/libRDKitSmilesParse.so.1.2020.03.5)
==11551==by 0x4E63828:
RDKit::SmilesToMol(std::__cxx11::basic_string,
std::allocator > const&, RDKit::SmilesParserParams const&) (in
/usr/prog/s
cicomp/pythonds/develop/conda/lib/libRDKitSmilesParse.so.1.2020.03.5)
==11551==by 0x10D4B8:
RDKit::SmilesToMol(std::__cxx11::basic_string,
std::allocator > const&, int, bool,
std::map, std::allocator >,
std::__cxx11::basic_string,
std::allocator >, std::less, std::allocator > >,
std::allocator, std::allocator > const,
std::__cxx11::basic_string, std::allocator > > > >*)
(SmilesParse.h:79)
==11551==by 0x10B3DC: main (testmem.cpp:6)
==11551==
Cheers,
p.
On Thu, Aug 27, 2020 at 9:17 PM Jason Biggs wrote:
> Everything I know about C++ I learned just so that I can write a link
> between an interpreted language and the rdkit, so there are definitely some
> gaps in my knowledge.
>
> What I'm trying to understand right now is the expected lifetime of an
> Atom pointer returned by a molecule, for instance by the getAtomWithIdx
> method. Based on the documentation, since this method doesn't say the user
> is responsible for deleting the returned pointer I know I'm not supposed to
> delete it. But when exactly does it get deleted? If I dereference it after
> deleting the molecule, what is it?
>
> auto mol = RDKit::SmilesToMol("");
> auto atom = mol->getAtomWithIdx(0);
> auto m2 = atom->getOwningMol();
> std::cout << "Z=" << atom->getAtomicNum() << std::endl; // prints Z=6
> delete mol;
> std::cout << "Z=" << atom->getIdx() << std::endl; // prints Z=0
> std::cout << "N=" << m2.getNumAtoms() << std::endl;// prints N=4
> delete atom; // seg fault
>
> I would have thought the first time dereferencing the atom pointer after
> deleting mol would have crashed, but it does not. I would also have
> expected bad things when calling the getNumAtoms method on m2 after calling
> delete on mol, but this also works just fine. What am I missing?
>
> Thanks
> Jason
> ___
> Rdkit-discuss mailing list
> Rdkit-discuss@lists.sourceforge.net
>
Re: [Rdkit-discuss] c++ atomic lifetime
On 27/08/2020 20:15, Jason Biggs wrote:
Everything I know about C++ I learned just so that I can write a link
between an interpreted language and the rdkit, so there are definitely
some gaps in my knowledge.
What I'm trying to understand right now is the expected lifetime of an
Atom pointer returned by a molecule, for instance by the
getAtomWithIdx method. Based on the documentation, since this method
doesn't say the user is responsible for deleting the returned pointer
I know I'm not supposed to delete it. But when exactly does it get
deleted? If I dereference it after deleting the molecule, what is it?
auto mol = RDKit::SmilesToMol("");
auto atom = mol->getAtomWithIdx(0);
auto m2 = atom->getOwningMol();
std::cout << "Z=" << atom->getAtomicNum() << std::endl; // prints Z=6
delete mol;
std::cout << "Z=" << atom->getIdx() << std::endl; // prints Z=0
std::cout << "N=" << m2.getNumAtoms() << std::endl;// prints N=4
delete atom; // seg fault
I would have thought the first time dereferencing the atom pointer
after deleting mol would have crashed, but it does not. I would also
have expected bad things when calling the getNumAtoms method on m2
after calling delete on mol, but this also works just fine. What am I
missing?
Isn't this the soft of undefined behaviour that one would expect when
accessing deleted memory? Try adding some code between the deletion of
mol and the access of atom that allocates and deallocs some memory for a
second or so.
Anyway, I wouldn't try to "out-clever" the RDKit by deleting molecules
"by hand."
Paul.
___
Rdkit-discuss mailing list
Rdkit-discuss@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
On Thu, Aug 27, 2020 at 4:33 PM David Cosgrove
wrote:
> Hi Jason,
> The answer is that when you delete the molecule, the memory it uses is
> flagged as available for re-use, but nothing else happens to it. If you
> then de-reference pointers to it, such as the atoms that are buried in the
> block of memory allocated to the molecule, you may get away with it and you
> may not. It will depend on whether something else has written over the
> memory or not. In your example, the memory was still in its original state,
> so the de-referencing of the atom pointers succeeded. This is not
> guaranteed, however, and this sort of bug is generally very nasty to find-
> sometimes the code will run, sometimes it will crash. Worse still is if you
> accidentally write to de-allocated memory that something else is now using-
> you can then get failures 5 minutes later in a completely different part of
> the program.
>
Thank you David, this really helps. Thank you also to Dan, Nils, and
Dima.
I knew that accessing that atom after deleting the molecule was bad juju,
and was confused why it worked. I will steer clear of undefined behavior.
In my application, I have a wrapper class that I expose to top level users,
which holds a unique pointer to an ROMol. I know then that when my wrapper
class member goes away so does the ROMol. What I don't have is a similar
wrapper class for an Atom, precisely because of these ownership questions -
any atom properties or modifications go through the ROMol wrapper class.
I'm not very familiar with how the python interface works, is there a
similar issue with the python wrappers? Does the wrapper class for the
Atom clean up after itself differently if the atom is marked as having an
owner? Hope that question isn't too vague
Jason
>
> Deleting the atoms is also an error, because they will be deleted by the
> molecule’s destructor, so you’ll be de-allocating the memory twice, another
> exciting source of undefined behaviour. Valgrind is excellent for tracking
> down these sorts of error, and many more besides. If you’re developing on
> Linux, it’s good practice to use it on any code before you use that program
> in earnest.
>
> Cheers,
> Dave
>
>
> On Thu, 27 Aug 2020 at 20:17, Jason Biggs wrote:
>
>> Everything I know about C++ I learned just so that I can write a link
>> between an interpreted language and the rdkit, so there are definitely some
>> gaps in my knowledge.
>>
>> What I'm trying to understand right now is the expected lifetime of an
>> Atom pointer returned by a molecule, for instance by the getAtomWithIdx
>> method. Based on the documentation, since this method doesn't say the user
>> is responsible for deleting the returned pointer I know I'm not supposed to
>> delete it. But when exactly does it get deleted? If I dereference it after
>> deleting the molecule, what is it?
>>
>> auto mol = RDKit::SmilesToMol("");
>> auto atom = mol->getAtomWithIdx(0);
>> auto m2 = atom->getOwningMol();
>> std::cout << "Z=" << atom->getAtomicNum() << std::endl; // prints Z=6
>> delete mol;
>> std::cout << "Z=" << atom->getIdx() << std::endl; // prints Z=0
>> std::cout << "N=" << m2.getNumAtoms() << std::endl;// prints N=4
>> delete atom; // seg fault
>>
>> I would have thought the first time dereferencing the atom pointer after
>> deleting mol would have crashed, but it does not. I would also have
>> expected bad things when calling the getNumAtoms method on m2 after calling
>> delete on mol, but this also works just fine. What am I missing?
>>
>> Thanks
>> Jason
>>
>>
>> ___
>>
>> Rdkit-discuss mailing list
>>
>> Rdkit-discuss@lists.sourceforge.net
>>
>> https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
>>
>> --
> David Cosgrove
> Freelance computational chemistry and chemoinformatics developer
> http://cozchemix.co.uk
>
>
___
Rdkit-discuss mailing list
Rdkit-discuss@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
In RDKit, atoms are owned by the molecule. When you ask for:
auto atom = mol->getAtomWithIdx(0);
You are asking for a pointer to memory internally owned by the
RDKit::ROMol. However:
auto mol = RDKit::SmilesToMol("");
creates a new molecule in memory, so it's your job to delete it. This is
documented here:
https://github.com/rdkit/rdkit/blob/e86e2c1d5d375c75cbd7e00871ecc1e0a29b3548/Code/GraphMol/SmilesParse/SmilesParse.h#L47.
I think that, in general, RDKit tends to document when you *do* need to
clean up after yourself.
I'd recommend one of these idioms:
ROMOL_SPTR mol1(RDKit::SmilesToMol("")); // this is a
boost::shared_ptr, requires #include
std::unique_ptr mol2(RDKit::SmilesToMol("")); // requires
#include
*dan nealschneider* | lead developer
[image: Schrodinger Logo]
Re: [Rdkit-discuss] c++ atomic lifetime
Hi Jason,
The answer is that when you delete the molecule, the memory it uses is
flagged as available for re-use, but nothing else happens to it. If you
then de-reference pointers to it, such as the atoms that are buried in the
block of memory allocated to the molecule, you may get away with it and you
may not. It will depend on whether something else has written over the
memory or not. In your example, the memory was still in its original state,
so the de-referencing of the atom pointers succeeded. This is not
guaranteed, however, and this sort of bug is generally very nasty to find-
sometimes the code will run, sometimes it will crash. Worse still is if you
accidentally write to de-allocated memory that something else is now using-
you can then get failures 5 minutes later in a completely different part of
the program.
Deleting the atoms is also an error, because they will be deleted by the
molecule’s destructor, so you’ll be de-allocating the memory twice, another
exciting source of undefined behaviour. Valgrind is excellent for tracking
down these sorts of error, and many more besides. If you’re developing on
Linux, it’s good practice to use it on any code before you use that program
in earnest.
Cheers,
Dave
On Thu, 27 Aug 2020 at 20:17, Jason Biggs wrote:
> Everything I know about C++ I learned just so that I can write a link
> between an interpreted language and the rdkit, so there are definitely some
> gaps in my knowledge.
>
> What I'm trying to understand right now is the expected lifetime of an
> Atom pointer returned by a molecule, for instance by the getAtomWithIdx
> method. Based on the documentation, since this method doesn't say the user
> is responsible for deleting the returned pointer I know I'm not supposed to
> delete it. But when exactly does it get deleted? If I dereference it after
> deleting the molecule, what is it?
>
> auto mol = RDKit::SmilesToMol("");
> auto atom = mol->getAtomWithIdx(0);
> auto m2 = atom->getOwningMol();
> std::cout << "Z=" << atom->getAtomicNum() << std::endl; // prints Z=6
> delete mol;
> std::cout << "Z=" << atom->getIdx() << std::endl; // prints Z=0
> std::cout << "N=" << m2.getNumAtoms() << std::endl;// prints N=4
> delete atom; // seg fault
>
> I would have thought the first time dereferencing the atom pointer after
> deleting mol would have crashed, but it does not. I would also have
> expected bad things when calling the getNumAtoms method on m2 after calling
> delete on mol, but this also works just fine. What am I missing?
>
> Thanks
> Jason
>
>
> ___
>
> Rdkit-discuss mailing list
>
> Rdkit-discuss@lists.sourceforge.net
>
> https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
>
> --
David Cosgrove
Freelance computational chemistry and chemoinformatics developer
http://cozchemix.co.uk
___
Rdkit-discuss mailing list
Rdkit-discuss@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
On 8/27/2020 3:06 PM, Nils Weskamp wrote: To add to this: you are looking at the wonderful concept of an "undefined behavior" in C/C++. There is no guarantee that your example program will always show the same behaviour. In more recent versions of C++, you have access to "smart pointers" like std::shared_ptr, which basically implement reference counting. Not sure if this would help here. It's worse: with all the boost junk they pulled in the really recent versions, good luck figuring out which calls pass "smart" pointers and which don't. There are reasons why everyone's into Rust, and the efforts of C++ Standards Committee are behind many of them. Dima ___ Rdkit-discuss mailing list Rdkit-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
To add to this: you are looking at the wonderful concept of an "undefined behavior" in C/C++. There is no guarantee that your example program will always show the same behaviour. In more recent versions of C++, you have access to "smart pointers" like std::shared_ptr, which basically implement reference counting. Not sure if this would help here. Best regards, Nils Am 27.08.2020 um 21:42 schrieb dmaziuk via Rdkit-discuss: > On 8/27/2020 2:15 PM, Jason Biggs wrote: > >> What I'm trying to understand right now is the expected lifetime of an >> Atom >> pointer returned by a molecule, for instance by the getAtomWithIdx >> method. >> Based on the documentation, since this method doesn't say the user is >> responsible for deleting the returned pointer I know I'm not supposed to >> delete it. But when exactly does it get deleted? If I dereference it >> after >> deleting the molecule, what is it? > > The more general answer is: > > a) when the program terminates, all its resources are returned to the > OS. It was a common CGI technique to not bother and just let the it run > to the end. (It was also one of the "mobile Java" things with cellphone > vendors: they wanted garbage collection off.) > > b) Unlike "garbage-collected" languages c++ has guaranteed object > destruction. If there's any resources you want to explicitly relinquish, > the destructor is the place to do it. > > If your program is not an "up forever" server, you could just let it be: > it'll all get cleaned up on exit. > > HTH, > Dima > > > ___ > Rdkit-discuss mailing list > Rdkit-discuss@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/rdkit-discuss ___ Rdkit-discuss mailing list Rdkit-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
Re: [Rdkit-discuss] c++ atomic lifetime
On 8/27/2020 2:15 PM, Jason Biggs wrote: What I'm trying to understand right now is the expected lifetime of an Atom pointer returned by a molecule, for instance by the getAtomWithIdx method. Based on the documentation, since this method doesn't say the user is responsible for deleting the returned pointer I know I'm not supposed to delete it. But when exactly does it get deleted? If I dereference it after deleting the molecule, what is it? The more general answer is: a) when the program terminates, all its resources are returned to the OS. It was a common CGI technique to not bother and just let the it run to the end. (It was also one of the "mobile Java" things with cellphone vendors: they wanted garbage collection off.) b) Unlike "garbage-collected" languages c++ has guaranteed object destruction. If there's any resources you want to explicitly relinquish, the destructor is the place to do it. If your program is not an "up forever" server, you could just let it be: it'll all get cleaned up on exit. HTH, Dima ___ Rdkit-discuss mailing list Rdkit-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
[Rdkit-discuss] c++ atomic lifetime
Everything I know about C++ I learned just so that I can write a link
between an interpreted language and the rdkit, so there are definitely some
gaps in my knowledge.
What I'm trying to understand right now is the expected lifetime of an Atom
pointer returned by a molecule, for instance by the getAtomWithIdx method.
Based on the documentation, since this method doesn't say the user is
responsible for deleting the returned pointer I know I'm not supposed to
delete it. But when exactly does it get deleted? If I dereference it after
deleting the molecule, what is it?
auto mol = RDKit::SmilesToMol("");
auto atom = mol->getAtomWithIdx(0);
auto m2 = atom->getOwningMol();
std::cout << "Z=" << atom->getAtomicNum() << std::endl; // prints Z=6
delete mol;
std::cout << "Z=" << atom->getIdx() << std::endl; // prints Z=0
std::cout << "N=" << m2.getNumAtoms() << std::endl;// prints N=4
delete atom; // seg fault
I would have thought the first time dereferencing the atom pointer after
deleting mol would have crashed, but it does not. I would also have
expected bad things when calling the getNumAtoms method on m2 after calling
delete on mol, but this also works just fine. What am I missing?
Thanks
Jason
___
Rdkit-discuss mailing list
Rdkit-discuss@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
