Over-riding copy behaviour and group indices

17.02.2011, 20:51, “David Lonie” loniedavid@gmail.com:

On Thu, Feb 17, 2011 at 12:42 PM, Konstantin Tokarev annulen@yandex.ru; wrote:

17.02.2011, 19:05, “David Lonie” loniedavid@gmail.com;:

From a design standpoint, what would be the best way to handle this? I
understand that it’s a bad idea to lock Molecule::lock from inside a
member function, but calculateGroupIndices should really be called
inside of a read-lock. Since this is a slot called from a signal, it
doesn’t seem possible to read-lock this in an obvious way.

Perhaps a if(!lock()->tryLockForRead()){return;} at the beginning of
the function would be best?
I guess you propose tryLockForRead() because lockForRead() will
deadlock if write lock is acquired in the same thread. However it would
be better if calculateGroupIndices waited for unlock in case lock was
acquired in another thread. Any ideas?

Right – Molecule::lock should only be locked from outside of the
Molecule member functions for consistency. One approach I’ve used in
similar situations is:

v> oid memberSlot()
{
if (!memberLock()->tryLockForRead()) {
QTimer::singleShot(100, this, SLOT(memberSlot()));
return;
}

// … do work …

memberLock()->unlock();
}

But this will be horribly inefficient when dealing with large number
of additions/removals. It also behaves in an unexpected way when
calculateGroupIndices is called under a write-lock, since the index
assignment is delayed until the lock is released. I agree that a
caching/dirty-boolean approach is best, since a read-lock should be
used when accessing the indices anyway.

But danger of Molecule changing during calculateGroupIndices run
remains, especially if labelengine is used and group indicies are active.

Maybe it’s needed to run calculateGroupIndices with QtConcurrent::run,
and use lockForWrite inside? No deadlock, no possibility for changes
before calculation ends, always valid return value of Atom::groupIndex


Regards,
Konstantin

17.02.2011, 21:14, “Marcus D. Hanwell” mhanwell@gmail.com:

It would be great to figure out what is changing the molecule
mid-render. That should not happen.

Do you mean lock is not needed here?


Regards,
Konstantin

2011/2/17 Konstantin Tokarev annulen@yandex.ru:

17.02.2011, 20:51, “David Lonie” loniedavid@gmail.com:

On Thu, Feb 17, 2011 at 12:42 PM, Konstantin Tokarev annulen@yandex.ru; wrote:

17.02.2011, 19:05, “David Lonie” loniedavid@gmail.com;:

From a design standpoint, what would be the best way to handle this? I
understand that it’s a bad idea to lock Molecule::lock from inside a
member function, but calculateGroupIndices should really be called
inside of a read-lock. Since this is a slot called from a signal, it
doesn’t seem possible to read-lock this in an obvious way.

Perhaps a if(!lock()->tryLockForRead()){return;} at the beginning of
the function would be best?
I guess you propose tryLockForRead() because lockForRead() will
deadlock if write lock is acquired in the same thread. However it would
be better if calculateGroupIndices waited for unlock in case lock was
acquired in another thread. Any ideas?

Right – Molecule::lock should only be locked from outside of the
Molecule member functions for consistency. One approach I’ve used in
similar situations is:

void memberSlot()
{
if (!memberLock()->tryLockForRead()) {
QTimer::singleShot(100, this, SLOT(memberSlot()));
return;
}

// … do work …

memberLock()->unlock();
}

But this will be horribly inefficient when dealing with large number
of additions/removals. It also behaves in an unexpected way when
calculateGroupIndices is called under a write-lock, since the index
assignment is delayed until the lock is released. I agree that a
caching/dirty-boolean approach is best, since a read-lock should be
used when accessing the indices anyway.

But danger of Molecule changing during calculateGroupIndices run
remains, especially if labelengine is used and group indicies are active.

Not if labelengine accesses the group indices under a read-lock.

Maybe it’s needed to run calculateGroupIndices with QtConcurrent::run,
and use lockForWrite inside? No deadlock, no possibility for changes
before calculation ends, always valid return value of Atom::groupIndex

This has the same drawback as the QTimer::singleshot approach – the
call to calculateGroupIndices would return before the indices are
assigned, which is very unexpected. I suppose a QFutureWatcher could
be set up to notify when the assignment is complete, but that would be
unnecessarily complicated. I still favor the dirty boolean and only
calculating the indices on first access (and under a read-lock on the
molecule). I would also reduce the visibility of calculateGroupIndices
if this approach is taken (unless the function is old enough that this
would break API…).

Dave

On Thu, Feb 17, 2011 at 1:14 PM, Marcus D. Hanwell
mhanwell@gmail.com wrote:

On Thu, Feb 17, 2011 at 5:42 PM, Konstantin Tokarev annulen@yandex.ru wrote:

I guess you propose tryLockForRead() because lockForRead() will
deadlock if write lock is acquired in the same thread. However it would
be better if calculateGroupIndices waited for unlock in case lock was
acquired in another thread. Any ideas?

It would be great to figure out what is changing the molecule
mid-render. That should not happen.

The problematic call to calculateGroupIndices is triggered by
Molecule::updateAtom, most likely from atoms being added/removed. It’s
a separate issue from the call during rendering.

Dave

17.02.2011, 21:32, “David Lonie” loniedavid@gmail.com:

On Thu, Feb 17, 2011 at 1:14 PM, Marcus D. Hanwell
mhanwell@gmail.com; wrote:

On Thu, Feb 17, 2011 at 5:42 PM, Konstantin Tokarev annulen@yandex.ru; wrote:

I guess you propose tryLockForRead() because lockForRead() will
deadlock if write lock is acquired in the same thread. However it would
be better if calculateGroupIndices waited for unlock in case lock was
acquired in another thread. Any ideas?
It would be great to figure out what is changing the molecule
mid-render. That should not happen.

The problematic call to calculateGroupIndices is triggered by
Molecule::updateAtom, most likely from atoms being added/removed. It’s
a separate issue from the call during rendering

calculateGroupIndices is not called on molecule changes anymore


Regards,
Konstantin

On Thu, Feb 17, 2011 at 6:32 PM, David Lonie loniedavid@gmail.com wrote:

On Thu, Feb 17, 2011 at 1:14 PM, Marcus D. Hanwell
mhanwell@gmail.com wrote:

On Thu, Feb 17, 2011 at 5:42 PM, Konstantin Tokarev annulen@yandex.ru wrote:

I guess you propose tryLockForRead() because lockForRead() will
deadlock if write lock is acquired in the same thread. However it would
be better if calculateGroupIndices waited for unlock in case lock was
acquired in another thread. Any ideas?

It would be great to figure out what is changing the molecule
mid-render. That should not happen.

The problematic call to calculateGroupIndices is triggered by
Molecule::updateAtom, most likely from atoms being added/removed. It’s
a separate issue from the call during rendering.

I removed it from there as it was an inefficient implementation. If
you update then that should be done.

Marcus

2011/2/17 Konstantin Tokarev annulen@yandex.ru:

17.02.2011, 21:32, “David Lonie” loniedavid@gmail.com:

On Thu, Feb 17, 2011 at 1:14 PM, Marcus D. Hanwell
mhanwell@gmail.com; wrote:

On Thu, Feb 17, 2011 at 5:42 PM, Konstantin Tokarev annulen@yandex.ru; wrote:

I guess you propose tryLockForRead() because lockForRead() will
deadlock if write lock is acquired in the same thread. However it would
be better if calculateGroupIndices waited for unlock in case lock was
acquired in another thread. Any ideas?
It would be great to figure out what is changing the molecule
mid-render. That should not happen.

The problematic call to calculateGroupIndices is triggered by
Molecule::updateAtom, most likely from atoms being added/removed. It’s
a separate issue from the call during rendering

calculateGroupIndices is not called on molecule changes anymore

It gets called when an atomic number is changed, etc.

Dave

On Thu, Feb 17, 2011 at 1:38 PM, Marcus D. Hanwell
mhanwell@gmail.com wrote:

On Thu, Feb 17, 2011 at 6:32 PM, David Lonie loniedavid@gmail.com wrote:

On Thu, Feb 17, 2011 at 1:14 PM, Marcus D. Hanwell
mhanwell@gmail.com wrote:

On Thu, Feb 17, 2011 at 5:42 PM, Konstantin Tokarev annulen@yandex.ru wrote:

I guess you propose tryLockForRead() because lockForRead() will
deadlock if write lock is acquired in the same thread. However it would
be better if calculateGroupIndices waited for unlock in case lock was
acquired in another thread. Any ideas?

It would be great to figure out what is changing the molecule
mid-render. That should not happen.

The problematic call to calculateGroupIndices is triggered by
Molecule::updateAtom, most likely from atoms being added/removed. It’s
a separate issue from the call during rendering.

I removed it from there as it was an inefficient implementation. If
you update then that should be done.

It’s still there in cryos/master:

Which commit has the change?

Dave

17.02.2011, 21:45, “David Lonie” loniedavid@gmail.com:

On Thu, Feb 17, 2011 at 1:38 PM, Marcus D. Hanwell
mhanwell@gmail.com; wrote:

On Thu, Feb 17, 2011 at 6:32 PM, David Lonie loniedavid@gmail.com; wrote:

On Thu, Feb 17, 2011 at 1:14 PM, Marcus D. Hanwell
mhanwell@gmail.com; wrote:

On Thu, Feb 17, 2011 at 5:42 PM, Konstantin Tokarev annulen@yandex.ru; wrote:

I guess you propose tryLockForRead() because lockForRead() will
deadlock if write lock is acquired in the same thread. However it would
be better if calculateGroupIndices waited for unlock in case lock was
acquired in another thread. Any ideas?
It would be great to figure out what is changing the molecule
mid-render. That should not happen.
The problematic call to calculateGroupIndices is triggered by
Molecule::updateAtom, most likely from atoms being added/removed. It’s
a separate issue from the call during rendering.
I removed it from there as it was an inefficient implementation. If
you update then that should be done.

It’s still there in cryos/master:

avogadro/libavogadro/src/molecule.cpp at master · cryos/avogadro · GitHub

Which commit has the change?

c3b408880

But it was not removed from updateAtom(). I’ll take it


Regards,
Konstantin

17.02.2011, 21:30, “David Lonie” loniedavid@gmail.com:
I would also reduce the visibility of calculateGroupIndices

if this approach is taken (unless the function is old enough that this
would break API…).

It would not break API, but this function needs to be called from Atom
(Atom::groupIndex())

Or it’s better to replace Atom::groupIndex() with Molecule::groupIndex(Atom*)?
(I’d preferred not to do it)


Regards,
Konstantin

On Thu, Feb 17, 2011 at 2:04 PM, Konstantin Tokarev annulen@yandex.ru wrote:

17.02.2011, 21:30, “David Lonie” loniedavid@gmail.com:
I would also reduce the visibility of calculateGroupIndices

if this approach is taken (unless the function is old enough that this
would break API…).

It would not break API, but this function needs to be called from Atom
(Atom::groupIndex())

Or it’s better to replace Atom::groupIndex() with Molecule::groupIndex(Atom*)?
(I’d preferred not to do it)

Ah, ok. I’d personally make it a friend function, but leaving it
public wouldn’t really hurt anything – it’s just unnecessary if
groupIndex() calls it when needed.

Dave

17.02.2011, 22:12, “David Lonie” loniedavid@gmail.com:

On Thu, Feb 17, 2011 at 2:04 PM, Konstantin Tokarev annulen@yandex.ru; wrote:

17.02.2011, 21:30, “David Lonie” loniedavid@gmail.com;:
I would also reduce the visibility of calculateGroupIndices

if this approach is taken (unless the function is old enough that this
would break API…).
It would not break API, but this function needs to be called from Atom
(Atom::groupIndex())

Or it’s better to replace Atom::groupIndex() with Molecule::groupIndex(Atom*)?
(I’d preferred not to do it)

Ah, ok. I’d personally make it a friend function, but leaving it
public wouldn’t really hurt anything – it’s just unnecessary if
groupIndex() calls it when needed.

I’ve submitted patch to Gerrit


Regards,
Konstantin