Race conditions

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Race conditions

Noel O'Boyle
Administrator
Hi there,

Wouldn't a better solution to the race conditions that David Koes is
experiencing with global state be to remove the global state? For the
cases he mentioned, e.g. the OBAromTyper, the global state relating to
a single molecule can easily be moved to a OBAromTyperPrivate class
instantiated by a TypeThisMolecule() function in the global class.

This is an API breakage, but only because these internal
implementation functions were exposed. I think the time might be ripe
for a couple of API cleanups, not for the sake of it, but where they
limit or affect the toolkit's usage.

Regards,
- Noel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

David Koes
I think this is most definitely the way to go if you can tolerate the API changes.  The main drawback is potentially each molecule class taking up more memory.  I would welcome any changes that push openbabel closer to thread safety..

David Koes

Assistant Professor
Computational & Systems Biology
University of Pittsburgh


> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <[hidden email]> wrote:
>
> Hi there,
>
> Wouldn't a better solution to the race conditions that David Koes is
> experiencing with global state be to remove the global state? For the
> cases he mentioned, e.g. the OBAromTyper, the global state relating to
> a single molecule can easily be moved to a OBAromTyperPrivate class
> instantiated by a TypeThisMolecule() function in the global class.
>
> This is an API breakage, but only because these internal
> implementation functions were exposed. I think the time might be ripe
> for a couple of API cleanups, not for the sake of it, but where they
> limit or affect the toolkit's usage.
>
> Regards,
> - Noel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
There's no memory overhead on the molecule class - it's unchanged. Let
me make the change on a branch for one of the classes, and we can chew
it over.

- Noel

On 7 February 2017 at 13:35, Koes, David <[hidden email]> wrote:

> I think this is most definitely the way to go if you can tolerate the API changes.  The main drawback is potentially each molecule class taking up more memory.  I would welcome any changes that push openbabel closer to thread safety..
>
> David Koes
>
> Assistant Professor
> Computational & Systems Biology
> University of Pittsburgh
>
>
>> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <[hidden email]> wrote:
>>
>> Hi there,
>>
>> Wouldn't a better solution to the race conditions that David Koes is
>> experiencing with global state be to remove the global state? For the
>> cases he mentioned, e.g. the OBAromTyper, the global state relating to
>> a single molecule can easily be moved to a OBAromTyperPrivate class
>> instantiated by a TypeThisMolecule() function in the global class.
>>
>> This is an API breakage, but only because these internal
>> implementation functions were exposed. I think the time might be ripe
>> for a couple of API cleanups, not for the sake of it, but where they
>> limit or affect the toolkit's usage.
>>
>> Regards,
>> - Noel
>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: API break (was Race conditions)

Geoff Hutchison
In reply to this post by Noel O'Boyle
I would be just fine making 2017 the target for 3.0 and some level of API breaks and cleanups. There are already a pile of methods marked “deprecated” in the code.

I don’t think it’s worth changing atom indexing - but anything else should be on the table.

-Geoff

> This is an API breakage, but only because these internal
> implementation functions were exposed. I think the time might be ripe
> for a couple of API cleanups, not for the sake of it, but where they
> limit or affect the toolkit's usage.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: API break (was Race conditions)

Noel O'Boyle
Administrator
Great, because I wanted to talk about atom indexing. Just kidding....

However, I would like to replace the current usage of _impval as a structure invariant, with _imph instead. I think Chris looked into the possibility of doing this a long time ago. To my mind, this as the number one problem with the library (though I guess everyone has their favourite). I don't know if I need to go why I feel that way, but I'm happy to expand on this. As a teaser, see the code for OBAtom.ImplicitHydrogenCount(), which would be replaced with returning _imph.

I remember Chris saying that he felt that surprisingly few changes were necessary to make this happen. He may be right.

Regards,
- Noel


On 10 Feb 2017 6:20 p.m., "Geoffrey Hutchison" <[hidden email]> wrote:
I would be just fine making 2017 the target for 3.0 and some level of API breaks and cleanups. There are already a pile of methods marked “deprecated” in the code.

I don’t think it’s worth changing atom indexing - but anything else should be on the table.

-Geoff

> This is an API breakage, but only because these internal
> implementation functions were exposed. I think the time might be ripe
> for a couple of API cleanups, not for the sake of it, but where they
> limit or affect the toolkit's usage.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: API break (was Race conditions)

Geoff Hutchison
> I remember Chris saying that he felt that surprisingly few changes were necessary to make this happen. He may be right.

Sounds great. If you work on a branch, we can all take a look.

If you need help - send out a post. :-)

-Geoff


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
In reply to this post by Noel O'Boyle
Here's of what I was thinking:
https://github.com/baoilleach/openbabel/commit/697b92180f9f61f738ef3e5c04d30340b9f8ee8e

If it looks okay, I can make a similar change to other classes that
have the same problem (assuming one size fits all).

- Noel


On 8 February 2017 at 09:32, Noel O'Boyle <[hidden email]> wrote:

> There's no memory overhead on the molecule class - it's unchanged. Let
> me make the change on a branch for one of the classes, and we can chew
> it over.
>
> - Noel
>
> On 7 February 2017 at 13:35, Koes, David <[hidden email]> wrote:
>> I think this is most definitely the way to go if you can tolerate the API changes.  The main drawback is potentially each molecule class taking up more memory.  I would welcome any changes that push openbabel closer to thread safety..
>>
>> David Koes
>>
>> Assistant Professor
>> Computational & Systems Biology
>> University of Pittsburgh
>>
>>
>>> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <[hidden email]> wrote:
>>>
>>> Hi there,
>>>
>>> Wouldn't a better solution to the race conditions that David Koes is
>>> experiencing with global state be to remove the global state? For the
>>> cases he mentioned, e.g. the OBAromTyper, the global state relating to
>>> a single molecule can easily be moved to a OBAromTyperPrivate class
>>> instantiated by a TypeThisMolecule() function in the global class.
>>>
>>> This is an API breakage, but only because these internal
>>> implementation functions were exposed. I think the time might be ripe
>>> for a couple of API cleanups, not for the sake of it, but where they
>>> limit or affect the toolkit's usage.
>>>
>>> Regards,
>>> - Noel
>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
Classes or variables with a global instance are susceptible to
multithreading problems. It turns out that these are listed in the
Python bindings.

Classes: OBElementTable, OBTypeTable, OBIsotopeTable, OBAromaticTyper,
OBAtomTyper, OBChainsParser, OBResidueData, OBPhModel, OBErrorLog
Variables: Residue, ElemDesc, ResNo, ElemNo (these all from the bottom
of residue.h)

The following don't appear to be a problem:
OBElementTable, OBTypeTable, OBIsotopeTable

On my branch (https://github.com/baoilleach/openbabel/tree/fixglobalstate)
I think I've fixed the following:
OBAromaticTyper, OBAtomTyper, OBPhModel

Roger didn't anticipate multi-processor architectures and it looks
like OBChainsParser and OBResidueData as well as those variables
listed above do have a problem. It should be possible to fix, but
perceiving residues (or reading PDB and also I guess MOL2) will cause
problems right now.

I haven't really looked at OBErrorLog, and my time is up right now.
Have I missed any classes that you know of?

David, do you have any test code that reads an SDF multithreadedly,
that could be used to force segfaults?

Regards,
- Noel

On 11 February 2017 at 21:08, Noel O'Boyle <[hidden email]> wrote:

> Here's of what I was thinking:
> https://github.com/baoilleach/openbabel/commit/697b92180f9f61f738ef3e5c04d30340b9f8ee8e
>
> If it looks okay, I can make a similar change to other classes that
> have the same problem (assuming one size fits all).
>
> - Noel
>
>
> On 8 February 2017 at 09:32, Noel O'Boyle <[hidden email]> wrote:
>> There's no memory overhead on the molecule class - it's unchanged. Let
>> me make the change on a branch for one of the classes, and we can chew
>> it over.
>>
>> - Noel
>>
>> On 7 February 2017 at 13:35, Koes, David <[hidden email]> wrote:
>>> I think this is most definitely the way to go if you can tolerate the API changes.  The main drawback is potentially each molecule class taking up more memory.  I would welcome any changes that push openbabel closer to thread safety..
>>>
>>> David Koes
>>>
>>> Assistant Professor
>>> Computational & Systems Biology
>>> University of Pittsburgh
>>>
>>>
>>>> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <[hidden email]> wrote:
>>>>
>>>> Hi there,
>>>>
>>>> Wouldn't a better solution to the race conditions that David Koes is
>>>> experiencing with global state be to remove the global state? For the
>>>> cases he mentioned, e.g. the OBAromTyper, the global state relating to
>>>> a single molecule can easily be moved to a OBAromTyperPrivate class
>>>> instantiated by a TypeThisMolecule() function in the global class.
>>>>
>>>> This is an API breakage, but only because these internal
>>>> implementation functions were exposed. I think the time might be ripe
>>>> for a couple of API cleanups, not for the sake of it, but where they
>>>> limit or affect the toolkit's usage.
>>>>
>>>> Regards,
>>>> - Noel
>>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
...googling through the mailing list, I see that David Lonie mentions
that OBErrorLog has problems, and Tim that all formats are singletons.
I need to think some more about this latter.

- Noel

On 12 February 2017 at 16:45, Noel O'Boyle <[hidden email]> wrote:

> Classes or variables with a global instance are susceptible to
> multithreading problems. It turns out that these are listed in the
> Python bindings.
>
> Classes: OBElementTable, OBTypeTable, OBIsotopeTable, OBAromaticTyper,
> OBAtomTyper, OBChainsParser, OBResidueData, OBPhModel, OBErrorLog
> Variables: Residue, ElemDesc, ResNo, ElemNo (these all from the bottom
> of residue.h)
>
> The following don't appear to be a problem:
> OBElementTable, OBTypeTable, OBIsotopeTable
>
> On my branch (https://github.com/baoilleach/openbabel/tree/fixglobalstate)
> I think I've fixed the following:
> OBAromaticTyper, OBAtomTyper, OBPhModel
>
> Roger didn't anticipate multi-processor architectures and it looks
> like OBChainsParser and OBResidueData as well as those variables
> listed above do have a problem. It should be possible to fix, but
> perceiving residues (or reading PDB and also I guess MOL2) will cause
> problems right now.
>
> I haven't really looked at OBErrorLog, and my time is up right now.
> Have I missed any classes that you know of?
>
> David, do you have any test code that reads an SDF multithreadedly,
> that could be used to force segfaults?
>
> Regards,
> - Noel
>
> On 11 February 2017 at 21:08, Noel O'Boyle <[hidden email]> wrote:
>> Here's of what I was thinking:
>> https://github.com/baoilleach/openbabel/commit/697b92180f9f61f738ef3e5c04d30340b9f8ee8e
>>
>> If it looks okay, I can make a similar change to other classes that
>> have the same problem (assuming one size fits all).
>>
>> - Noel
>>
>>
>> On 8 February 2017 at 09:32, Noel O'Boyle <[hidden email]> wrote:
>>> There's no memory overhead on the molecule class - it's unchanged. Let
>>> me make the change on a branch for one of the classes, and we can chew
>>> it over.
>>>
>>> - Noel
>>>
>>> On 7 February 2017 at 13:35, Koes, David <[hidden email]> wrote:
>>>> I think this is most definitely the way to go if you can tolerate the API changes.  The main drawback is potentially each molecule class taking up more memory.  I would welcome any changes that push openbabel closer to thread safety..
>>>>
>>>> David Koes
>>>>
>>>> Assistant Professor
>>>> Computational & Systems Biology
>>>> University of Pittsburgh
>>>>
>>>>
>>>>> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <[hidden email]> wrote:
>>>>>
>>>>> Hi there,
>>>>>
>>>>> Wouldn't a better solution to the race conditions that David Koes is
>>>>> experiencing with global state be to remove the global state? For the
>>>>> cases he mentioned, e.g. the OBAromTyper, the global state relating to
>>>>> a single molecule can easily be moved to a OBAromTyperPrivate class
>>>>> instantiated by a TypeThisMolecule() function in the global class.
>>>>>
>>>>> This is an API breakage, but only because these internal
>>>>> implementation functions were exposed. I think the time might be ripe
>>>>> for a couple of API cleanups, not for the sake of it, but where they
>>>>> limit or affect the toolkit's usage.
>>>>>
>>>>> Regards,
>>>>> - Noel
>>>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
I've made progress, but my current problem is that it's not possible
to match the same SmartsPattern on different threads (as required, for
example, by the typers, where sets of smarts patterns are initialised
once and then reused). The matchers store the results as part of the
pattern instead of returning it directly, so two simultaneous matches
against the same pattern will confuse things. I'm not sure of the
exact details of a solution until I study the existing code, but it
will involve separating the parsed smarts pattern data structure from
any match results and I'm guessing this will need an API change.

- Noel

On 12 February 2017 at 20:18, Noel O'Boyle <[hidden email]> wrote:

> ...googling through the mailing list, I see that David Lonie mentions
> that OBErrorLog has problems, and Tim that all formats are singletons.
> I need to think some more about this latter.
>
> - Noel
>
> On 12 February 2017 at 16:45, Noel O'Boyle <[hidden email]> wrote:
>> Classes or variables with a global instance are susceptible to
>> multithreading problems. It turns out that these are listed in the
>> Python bindings.
>>
>> Classes: OBElementTable, OBTypeTable, OBIsotopeTable, OBAromaticTyper,
>> OBAtomTyper, OBChainsParser, OBResidueData, OBPhModel, OBErrorLog
>> Variables: Residue, ElemDesc, ResNo, ElemNo (these all from the bottom
>> of residue.h)
>>
>> The following don't appear to be a problem:
>> OBElementTable, OBTypeTable, OBIsotopeTable
>>
>> On my branch (https://github.com/baoilleach/openbabel/tree/fixglobalstate)
>> I think I've fixed the following:
>> OBAromaticTyper, OBAtomTyper, OBPhModel
>>
>> Roger didn't anticipate multi-processor architectures and it looks
>> like OBChainsParser and OBResidueData as well as those variables
>> listed above do have a problem. It should be possible to fix, but
>> perceiving residues (or reading PDB and also I guess MOL2) will cause
>> problems right now.
>>
>> I haven't really looked at OBErrorLog, and my time is up right now.
>> Have I missed any classes that you know of?
>>
>> David, do you have any test code that reads an SDF multithreadedly,
>> that could be used to force segfaults?
>>
>> Regards,
>> - Noel
>>
>> On 11 February 2017 at 21:08, Noel O'Boyle <[hidden email]> wrote:
>>> Here's of what I was thinking:
>>> https://github.com/baoilleach/openbabel/commit/697b92180f9f61f738ef3e5c04d30340b9f8ee8e
>>>
>>> If it looks okay, I can make a similar change to other classes that
>>> have the same problem (assuming one size fits all).
>>>
>>> - Noel
>>>
>>>
>>> On 8 February 2017 at 09:32, Noel O'Boyle <[hidden email]> wrote:
>>>> There's no memory overhead on the molecule class - it's unchanged. Let
>>>> me make the change on a branch for one of the classes, and we can chew
>>>> it over.
>>>>
>>>> - Noel
>>>>
>>>> On 7 February 2017 at 13:35, Koes, David <[hidden email]> wrote:
>>>>> I think this is most definitely the way to go if you can tolerate the API changes.  The main drawback is potentially each molecule class taking up more memory.  I would welcome any changes that push openbabel closer to thread safety..
>>>>>
>>>>> David Koes
>>>>>
>>>>> Assistant Professor
>>>>> Computational & Systems Biology
>>>>> University of Pittsburgh
>>>>>
>>>>>
>>>>>> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <[hidden email]> wrote:
>>>>>>
>>>>>> Hi there,
>>>>>>
>>>>>> Wouldn't a better solution to the race conditions that David Koes is
>>>>>> experiencing with global state be to remove the global state? For the
>>>>>> cases he mentioned, e.g. the OBAromTyper, the global state relating to
>>>>>> a single molecule can easily be moved to a OBAromTyperPrivate class
>>>>>> instantiated by a TypeThisMolecule() function in the global class.
>>>>>>
>>>>>> This is an API breakage, but only because these internal
>>>>>> implementation functions were exposed. I think the time might be ripe
>>>>>> for a couple of API cleanups, not for the sake of it, but where they
>>>>>> limit or affect the toolkit's usage.
>>>>>>
>>>>>> Regards,
>>>>>> - Noel
>>>>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
I've just realised that David Koes has already fixed this, by
providing just such an interface, a Match function where you pass in
the vector for storing the matches. I owe you a pint or two, David.

Note to self: as part of the API cleanup, we should remove the other functions.

- Noel

On 19 February 2017 at 10:21, Noel O'Boyle <[hidden email]> wrote:

> I've made progress, but my current problem is that it's not possible
> to match the same SmartsPattern on different threads (as required, for
> example, by the typers, where sets of smarts patterns are initialised
> once and then reused). The matchers store the results as part of the
> pattern instead of returning it directly, so two simultaneous matches
> against the same pattern will confuse things. I'm not sure of the
> exact details of a solution until I study the existing code, but it
> will involve separating the parsed smarts pattern data structure from
> any match results and I'm guessing this will need an API change.
>
> - Noel
>
> On 12 February 2017 at 20:18, Noel O'Boyle <[hidden email]> wrote:
>> ...googling through the mailing list, I see that David Lonie mentions
>> that OBErrorLog has problems, and Tim that all formats are singletons.
>> I need to think some more about this latter.
>>
>> - Noel
>>
>> On 12 February 2017 at 16:45, Noel O'Boyle <[hidden email]> wrote:
>>> Classes or variables with a global instance are susceptible to
>>> multithreading problems. It turns out that these are listed in the
>>> Python bindings.
>>>
>>> Classes: OBElementTable, OBTypeTable, OBIsotopeTable, OBAromaticTyper,
>>> OBAtomTyper, OBChainsParser, OBResidueData, OBPhModel, OBErrorLog
>>> Variables: Residue, ElemDesc, ResNo, ElemNo (these all from the bottom
>>> of residue.h)
>>>
>>> The following don't appear to be a problem:
>>> OBElementTable, OBTypeTable, OBIsotopeTable
>>>
>>> On my branch (https://github.com/baoilleach/openbabel/tree/fixglobalstate)
>>> I think I've fixed the following:
>>> OBAromaticTyper, OBAtomTyper, OBPhModel
>>>
>>> Roger didn't anticipate multi-processor architectures and it looks
>>> like OBChainsParser and OBResidueData as well as those variables
>>> listed above do have a problem. It should be possible to fix, but
>>> perceiving residues (or reading PDB and also I guess MOL2) will cause
>>> problems right now.
>>>
>>> I haven't really looked at OBErrorLog, and my time is up right now.
>>> Have I missed any classes that you know of?
>>>
>>> David, do you have any test code that reads an SDF multithreadedly,
>>> that could be used to force segfaults?
>>>
>>> Regards,
>>> - Noel
>>>
>>> On 11 February 2017 at 21:08, Noel O'Boyle <[hidden email]> wrote:
>>>> Here's of what I was thinking:
>>>> https://github.com/baoilleach/openbabel/commit/697b92180f9f61f738ef3e5c04d30340b9f8ee8e
>>>>
>>>> If it looks okay, I can make a similar change to other classes that
>>>> have the same problem (assuming one size fits all).
>>>>
>>>> - Noel
>>>>
>>>>
>>>> On 8 February 2017 at 09:32, Noel O'Boyle <[hidden email]> wrote:
>>>>> There's no memory overhead on the molecule class - it's unchanged. Let
>>>>> me make the change on a branch for one of the classes, and we can chew
>>>>> it over.
>>>>>
>>>>> - Noel
>>>>>
>>>>> On 7 February 2017 at 13:35, Koes, David <[hidden email]> wrote:
>>>>>> I think this is most definitely the way to go if you can tolerate the API changes.  The main drawback is potentially each molecule class taking up more memory.  I would welcome any changes that push openbabel closer to thread safety..
>>>>>>
>>>>>> David Koes
>>>>>>
>>>>>> Assistant Professor
>>>>>> Computational & Systems Biology
>>>>>> University of Pittsburgh
>>>>>>
>>>>>>
>>>>>>> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <[hidden email]> wrote:
>>>>>>>
>>>>>>> Hi there,
>>>>>>>
>>>>>>> Wouldn't a better solution to the race conditions that David Koes is
>>>>>>> experiencing with global state be to remove the global state? For the
>>>>>>> cases he mentioned, e.g. the OBAromTyper, the global state relating to
>>>>>>> a single molecule can easily be moved to a OBAromTyperPrivate class
>>>>>>> instantiated by a TypeThisMolecule() function in the global class.
>>>>>>>
>>>>>>> This is an API breakage, but only because these internal
>>>>>>> implementation functions were exposed. I think the time might be ripe
>>>>>>> for a couple of API cleanups, not for the sake of it, but where they
>>>>>>> limit or affect the toolkit's usage.
>>>>>>>
>>>>>>> Regards,
>>>>>>> - Noel
>>>>>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
After making the changes to use the alternative thread-safe Match
function, the next failure is with obLocale which is another global.
Commenting out "free (d->old_locale_string);" semi-works around it for
now (with a memory leak) but it can still segfault elsewhere in the
locale code.

Does anyone know exactly what problem the locale stuff is fixing? And
a follow-on, is there another way to fix it?

I have a test program at
https://gist.github.com/baoilleach/1a7e517798804bdd96b3e9b4927092ce
that reads and writes SMILES strings on multiple threads (with a
Windows-specific API). After all of the changes on the pull request,
it can now take quite a while before segfaulting (mostly in locale,
but worryingly once in NewAtom). Progress of sorts, I guess :-)

I think I'm done for the moment with this.

- Noel

On 19 February 2017 at 10:47, Noel O'Boyle <[hidden email]> wrote:

> I've just realised that David Koes has already fixed this, by
> providing just such an interface, a Match function where you pass in
> the vector for storing the matches. I owe you a pint or two, David.
>
> Note to self: as part of the API cleanup, we should remove the other functions.
>
> - Noel
>
> On 19 February 2017 at 10:21, Noel O'Boyle <[hidden email]> wrote:
>> I've made progress, but my current problem is that it's not possible
>> to match the same SmartsPattern on different threads (as required, for
>> example, by the typers, where sets of smarts patterns are initialised
>> once and then reused). The matchers store the results as part of the
>> pattern instead of returning it directly, so two simultaneous matches
>> against the same pattern will confuse things. I'm not sure of the
>> exact details of a solution until I study the existing code, but it
>> will involve separating the parsed smarts pattern data structure from
>> any match results and I'm guessing this will need an API change.
>>
>> - Noel
>>
>> On 12 February 2017 at 20:18, Noel O'Boyle <[hidden email]> wrote:
>>> ...googling through the mailing list, I see that David Lonie mentions
>>> that OBErrorLog has problems, and Tim that all formats are singletons.
>>> I need to think some more about this latter.
>>>
>>> - Noel
>>>
>>> On 12 February 2017 at 16:45, Noel O'Boyle <[hidden email]> wrote:
>>>> Classes or variables with a global instance are susceptible to
>>>> multithreading problems. It turns out that these are listed in the
>>>> Python bindings.
>>>>
>>>> Classes: OBElementTable, OBTypeTable, OBIsotopeTable, OBAromaticTyper,
>>>> OBAtomTyper, OBChainsParser, OBResidueData, OBPhModel, OBErrorLog
>>>> Variables: Residue, ElemDesc, ResNo, ElemNo (these all from the bottom
>>>> of residue.h)
>>>>
>>>> The following don't appear to be a problem:
>>>> OBElementTable, OBTypeTable, OBIsotopeTable
>>>>
>>>> On my branch (https://github.com/baoilleach/openbabel/tree/fixglobalstate)
>>>> I think I've fixed the following:
>>>> OBAromaticTyper, OBAtomTyper, OBPhModel
>>>>
>>>> Roger didn't anticipate multi-processor architectures and it looks
>>>> like OBChainsParser and OBResidueData as well as those variables
>>>> listed above do have a problem. It should be possible to fix, but
>>>> perceiving residues (or reading PDB and also I guess MOL2) will cause
>>>> problems right now.
>>>>
>>>> I haven't really looked at OBErrorLog, and my time is up right now.
>>>> Have I missed any classes that you know of?
>>>>
>>>> David, do you have any test code that reads an SDF multithreadedly,
>>>> that could be used to force segfaults?
>>>>
>>>> Regards,
>>>> - Noel
>>>>
>>>> On 11 February 2017 at 21:08, Noel O'Boyle <[hidden email]> wrote:
>>>>> Here's of what I was thinking:
>>>>> https://github.com/baoilleach/openbabel/commit/697b92180f9f61f738ef3e5c04d30340b9f8ee8e
>>>>>
>>>>> If it looks okay, I can make a similar change to other classes that
>>>>> have the same problem (assuming one size fits all).
>>>>>
>>>>> - Noel
>>>>>
>>>>>
>>>>> On 8 February 2017 at 09:32, Noel O'Boyle <[hidden email]> wrote:
>>>>>> There's no memory overhead on the molecule class - it's unchanged. Let
>>>>>> me make the change on a branch for one of the classes, and we can chew
>>>>>> it over.
>>>>>>
>>>>>> - Noel
>>>>>>
>>>>>> On 7 February 2017 at 13:35, Koes, David <[hidden email]> wrote:
>>>>>>> I think this is most definitely the way to go if you can tolerate the API changes.  The main drawback is potentially each molecule class taking up more memory.  I would welcome any changes that push openbabel closer to thread safety..
>>>>>>>
>>>>>>> David Koes
>>>>>>>
>>>>>>> Assistant Professor
>>>>>>> Computational & Systems Biology
>>>>>>> University of Pittsburgh
>>>>>>>
>>>>>>>
>>>>>>>> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <[hidden email]> wrote:
>>>>>>>>
>>>>>>>> Hi there,
>>>>>>>>
>>>>>>>> Wouldn't a better solution to the race conditions that David Koes is
>>>>>>>> experiencing with global state be to remove the global state? For the
>>>>>>>> cases he mentioned, e.g. the OBAromTyper, the global state relating to
>>>>>>>> a single molecule can easily be moved to a OBAromTyperPrivate class
>>>>>>>> instantiated by a TypeThisMolecule() function in the global class.
>>>>>>>>
>>>>>>>> This is an API breakage, but only because these internal
>>>>>>>> implementation functions were exposed. I think the time might be ripe
>>>>>>>> for a couple of API cleanups, not for the sake of it, but where they
>>>>>>>> limit or affect the toolkit's usage.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> - Noel
>>>>>>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Geoff Hutchison
> Does anyone know exactly what problem the locale stuff is fixing? And
> a follow-on, is there another way to fix it?

The problem is that we parse things like decimals for coordinates (0.0000) which exist in the C locale. So we need to make sure the parsing code works in C locale not any other locale.

> Windows-specific API). After all of the changes on the pull request,
> it can now take quite a while before segfaulting (mostly in locale,
> but worryingly once in NewAtom). Progress of sorts, I guess :-)

I'll take a look this week. Using Clang or g++, you should be able to use ThreadSanitizer to check:
https://clang.llvm.org/docs/ThreadSanitizer.html
https://github.com/google/sanitizers/wiki

Hope that helps,
-Geoff
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
On 19 February 2017 at 21:01, Geoffrey Hutchison
<[hidden email]> wrote:
>> Does anyone know exactly what problem the locale stuff is fixing? And
>> a follow-on, is there another way to fix it?
>
> The problem is that we parse things like decimals for coordinates (0.0000) which exist in the C locale. So we need to make sure the parsing code works in C locale not any other locale.

Gotcha. It does make me wonder whether we could avoid some of these
problems by parsing with C-style functions rather than C++ stream
based ones. I don't know the answer to that, nor whether it would be
feasible to change the existing codebase. On the other hand, such
problems might be easy to test/detect by setting the locale and seeing
whether the various formats give different results.

BTW, there's an additional complexitiy to consider with get/set
locale. By default (at least with MSVC on Win) it's per-process, so if
one thread get/sets the locale, it will affect the locale on the other
threads. However, there are commands to change this behaviour to make
it thread-locale (if you pardon the pun).

>> Windows-specific API). After all of the changes on the pull request,
>> it can now take quite a while before segfaulting (mostly in locale,
>> but worryingly once in NewAtom). Progress of sorts, I guess :-)
>
> I'll take a look this week. Using Clang or g++, you should be able to use ThreadSanitizer to check:
> https://clang.llvm.org/docs/ThreadSanitizer.html
> https://github.com/google/sanitizers/wiki

That's looks neat. I've already become a convert to AddressSanitizer
(not that I've ever needed it of course).

> Hope that helps,
> -Geoff

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Race conditions

Noel O'Boyle
Administrator
Actually, I think I've sorted it now, without any changes to OB. I
realised that obLocale reference counts (like BeginModify) and does
nothing if the locale is already set. See
https://gist.github.com/baoilleach/1a7e517798804bdd96b3e9b4927092ce

My earlier mention of a segfault within OBMol was probably a red
herring, an artifact of my use of the debugger where I may have jumped
between threads.

A few problems still remain, like in chains.cpp, but the main path
_seems_ sorted.

- Noel

On 20 February 2017 at 12:52, Noel O'Boyle <[hidden email]> wrote:

> On 19 February 2017 at 21:01, Geoffrey Hutchison
> <[hidden email]> wrote:
>>> Does anyone know exactly what problem the locale stuff is fixing? And
>>> a follow-on, is there another way to fix it?
>>
>> The problem is that we parse things like decimals for coordinates (0.0000) which exist in the C locale. So we need to make sure the parsing code works in C locale not any other locale.
>
> Gotcha. It does make me wonder whether we could avoid some of these
> problems by parsing with C-style functions rather than C++ stream
> based ones. I don't know the answer to that, nor whether it would be
> feasible to change the existing codebase. On the other hand, such
> problems might be easy to test/detect by setting the locale and seeing
> whether the various formats give different results.
>
> BTW, there's an additional complexitiy to consider with get/set
> locale. By default (at least with MSVC on Win) it's per-process, so if
> one thread get/sets the locale, it will affect the locale on the other
> threads. However, there are commands to change this behaviour to make
> it thread-locale (if you pardon the pun).
>
>>> Windows-specific API). After all of the changes on the pull request,
>>> it can now take quite a while before segfaulting (mostly in locale,
>>> but worryingly once in NewAtom). Progress of sorts, I guess :-)
>>
>> I'll take a look this week. Using Clang or g++, you should be able to use ThreadSanitizer to check:
>> https://clang.llvm.org/docs/ThreadSanitizer.html
>> https://github.com/google/sanitizers/wiki
>
> That's looks neat. I've already become a convert to AddressSanitizer
> (not that I've ever needed it of course).
>
>> Hope that helps,
>> -Geoff

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
Loading...