[vtk-developers] Change in VTK[master]: Added support for atomic integers.

[Berk Geveci]

> >Keep in mind that we want to support lots of platforms from various
> >desktops to supercomputers to potentially embedded systems.
>
> All the more reason to use std::atomic, no?  It's new, so isn't widely
supported yet, but going forward it seems preferable.  Newest gcc and clang
both claim
> to fully support C++11 now.

Very good point. I'll read up on std::atomic and add it as the main option
when not using OpenMP 3.1, TBB or Kaapi. I would still rather match the
atomic implementation to the parallel library when possible. It doesn't
sound like clang support std::atomic btw. Neither does it support other
atomic implementations. So it would fall back to the Mac OS functions.

> >> Was the OSAtomicIncrement32/64Barrier() call deliberately weakened to
> >> OSAtomicIncrement32/64()?  The Windows InterlockIncrement() is
documented
> >to use a
> >> memory barrier, and VTK on OS X did before, so...
> >
> >It was deliberate change based on the documentation, which says:
> >
> > *SNIP*
> >
> >What we are doing in vtkTimeStamp falls into "simply incrementing a
global
> >counter", which justifies using a much faster implementation in my
opinion.
> >What do you think? We'll probably have to revisit what AtomicInts do
under
> >covers if people start using them for other purposes than counters.
>
> Well, I am not a multithreading expert, but I know enough to know that
it's playing with fire. :)  The docs you  quote do say "Most code should
use the barrier
> functions to ensure that memory shared between threads is properly
synchronized".

I don't think that it is as bad as they make it sound. They are encouraging
you to be conservative. Everything should be OK as long as you are not
trying to use vtkTimeStamp to order the execution of multiple threads. A
common design pattern (the one they are referring to) is something like
this:

Atomic int foo set to 0

Thread A:

Initialize data structure X
Atomically increment foo

Thread B:

Wait until foo is 1
Use data structure X

If you don't use the Barrier version of the function, there is no guarantee
that the initialization of X will be done before foo is set to 1, which
makes what thread B does unsafe. The Barrier version guarantees it.

Given how vtkTimeStamp is designed, it would be foolish to use it for a
purpose like this. The following would work fine without the Barrier:

Thread A and B:

vtkNew<vtkObjectTypeFoo> obj;
mtime = obj.GetMTime();

// do something that might change obj

if (obj.GetMTime() > mtime)
   // do something else

As far as I know, this is the only design pattern the time stamp is
designed for. Am I missing something?

> If you think the weaker non-barrier version is sufficient, why on Windows
do you use InterlockedIncrement() and not InterlockedIncrementNoFence()?

Because I didn't know about InterlockedIncrementNoFence() :-)

Having said all of the above, I am leaning more towards changing to the
Barrier version now. The performance difference does not seem to be
significant (I will test more) and I can't foresee how people will use
AtomicInts in the future. By the way, std::atomic seems to be much more
thorough about it:

http://en.cppreference.com/w/cpp/atomic/memory_order

It sounds like the Barrier versions are equivalent
to std::memory_order_seq_cst.

The GCC ones also use full barriers.

-berk
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20130820/132c8498/attachment.html>