You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by Marcel Kornacker <ma...@cloudera.com> on 2016/12/08 11:10:03 UTC
AtomicInt vs. std::atomic
We have an AtomicInt in common/atomic.h that's based on some gutil
libraries. Is there any reason to keep this instead of switching to
std::atomic?
Re: AtomicInt vs. std::atomic
Posted by Daniel Hecht <dh...@cloudera.com>.
Agree with Henry and Todd. At least in the past, compilers have been
pretty bad at implementing atomics.
In addition, I'd prefer if we don't use std::atomic directly because it
uses operator overloading. Code involving atomics is much easier to
understand if the code itself shows the atomic operations, rather than
relying only on variable declarations. Of course, now that we've switched
to C++11, we could implement AtomicInt using std:atomic rather than gutil
(if std::atomic proves to be robust).
On Thu, Dec 8, 2016 at 5:20 AM, Todd Lipcon <to...@cloudera.com> wrote:
> We looked at it briefly for Kudu and decided not to use it for
> perf-sensitive cases at the time, based on the experience of Chromium. See
> https://chromium-cpp.appspot.com/ under the listing for atomics.
>
> That said, I believe there's been some work in LLVM in later versions to do
> some smarter optimizations of atomics that wouldn't be possible given the
> volatile asm blocks that Impala and Kudu use today. In particular, volatile
> asm prevents reordering of operations in both directions, whereas
> Release/Store semantics should only prevent reordering in one. So, it may
> be worth a shot, but worth testing carefully.
>
> -Todd
>
> On Thu, Dec 8, 2016 at 7:45 PM, Henry Robinson <he...@cloudera.com> wrote:
>
> > Not as far as I know. AtomicInt etc. were added at a time when Impala
> > didn't support C++11.
> >
> > On 8 December 2016 at 03:10, Marcel Kornacker <ma...@cloudera.com>
> wrote:
> >
> > > We have an AtomicInt in common/atomic.h that's based on some gutil
> > > libraries. Is there any reason to keep this instead of switching to
> > > std::atomic?
> > >
> >
> >
> >
> > --
> > Henry Robinson
> > Software Engineer
> > Cloudera
> > 415-994-6679
> >
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>
Re: AtomicInt vs. std::atomic
Posted by Todd Lipcon <to...@cloudera.com>.
We looked at it briefly for Kudu and decided not to use it for
perf-sensitive cases at the time, based on the experience of Chromium. See
https://chromium-cpp.appspot.com/ under the listing for atomics.
That said, I believe there's been some work in LLVM in later versions to do
some smarter optimizations of atomics that wouldn't be possible given the
volatile asm blocks that Impala and Kudu use today. In particular, volatile
asm prevents reordering of operations in both directions, whereas
Release/Store semantics should only prevent reordering in one. So, it may
be worth a shot, but worth testing carefully.
-Todd
On Thu, Dec 8, 2016 at 7:45 PM, Henry Robinson <he...@cloudera.com> wrote:
> Not as far as I know. AtomicInt etc. were added at a time when Impala
> didn't support C++11.
>
> On 8 December 2016 at 03:10, Marcel Kornacker <ma...@cloudera.com> wrote:
>
> > We have an AtomicInt in common/atomic.h that's based on some gutil
> > libraries. Is there any reason to keep this instead of switching to
> > std::atomic?
> >
>
>
>
> --
> Henry Robinson
> Software Engineer
> Cloudera
> 415-994-6679
>
--
Todd Lipcon
Software Engineer, Cloudera
Re: AtomicInt vs. std::atomic
Posted by Henry Robinson <he...@cloudera.com>.
Not as far as I know. AtomicInt etc. were added at a time when Impala
didn't support C++11.
On 8 December 2016 at 03:10, Marcel Kornacker <ma...@cloudera.com> wrote:
> We have an AtomicInt in common/atomic.h that's based on some gutil
> libraries. Is there any reason to keep this instead of switching to
> std::atomic?
>
--
Henry Robinson
Software Engineer
Cloudera
415-994-6679