Rewrite SamplingCircularQueue

Rewrite SamplingCircularQueue

The new implementation:
* uses MemoryBarriers to make sure up-to-date data is accessed on both producer and consumer threads
* will not allow to overwrite records
* doesn't have notion of chunks, instead each entry is aligned on the cache line boundaries

BUG=v8:2814
R=bmeurer@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16284

[yurys, 2013-08-12 14:27:46]

Benedikt, can you take a look?

[yurys, 2013-08-12 15:16:05]

I did some perf measurements on 3 devices(HP z620, Galaxy Nexus and Lenovo T60p)
with the deep stacks test attached to the bug and didn't see any slowdown when
using the new implementation. Results can be found here
https://docs.google.com/a/google.com/spreadsheet/pub?key=0AqSLK6jsEnwldDZzalB...

[Benedikt Meurer, 2013-08-13 09:31:32]

I don't think the new implementation actually improves the situation. The case
for multiple producers is still not handled correctly. We need to cleanup this
whole thing. See my comments below for suggestions how to proceed. CC'ing Sven
for additional feedback.

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue-inl.h
File src/circular-queue-inl.h (right):

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue-inl.h#n...
src/circular-queue-inl.h:50: }
This assumes that there is always only a single producer, but this is not always
the case, for example in Chrome on Linux with profiled web worker threads, and
maybe some other scenarios. We should get this right now while we're at it.

I'd suggest to use a tri-state scheme with kEmpty, kPreparing and kFull, and
turn the enqueue_pos into an AtomicWord. Then the code would look like this:

T* StartEnqueue() {
  Atomic32 curr = Acquire_Load(&enqueue_pos_);
  if (!Acquire_CompareAndSwap(&buffer_[curr + kRecordStateOffset], kEmpty,
kPreparing) != kEmpty) return NULL;
  Atomic32 next = (curr + kRecordSize) % (kRecordSize * N);
  ASSERT(Release_Load(&enqueue_pos_) == curr);
  Release_Store(&enqueue_pos_, next);
  return reinterpret_cast<T*>(&buffer_[curr]);
}

void FinishEnqueue(T* obj) {
  ASSERT(reinterpret_cast<Atomic32*>(obj) >= &buffer_[0] &&
              reinterpret_cast<Atomic32*>(obj) < &buffer_[kRecordSize * N]);
  Atomic32* state_ptr = reinterpret_cast<Atomic32*>(obj) + kRecordStateOffset;
  ASSERT(Release_Load(state_ptr) == kPreparing);
  Release_Store(state_ptr, kFull);
}

This way the objects are aligned on cache line boundaries, which is probably
better performancewise (guessing). If that turns out to be wrong, put the state
before the object again.

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc
File src/circular-queue.cc (right):

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc#newc...
src/circular-queue.cc:81: MemoryBarrier();
Why do we need this memory barrier here?

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc#newc...
src/circular-queue.cc:94: }
T* StartDequeue() {
  if (Acquire_Load(&buffer_[dequeue_pos_ + kRecordStateOffset] != kFull) return
NULL;
  return reinterpret_cast<T*>(&buffer_[dequeue_pos_]);
}

void FinishDequeue() {
  ASSERT(Release_Load(&buffer_[dequeue_pos_ + kRecordStateOffset] == kFull);
  Release_Store(&buffer_[dequeue_pos_ + kRecordStateOffset], kEmpty);
  dequeue_pos_ = (dequeue_pos + kRecordSize) % (N * kRecordSize);
}

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.h
File src/circular-queue.h (right):

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.h#newco...
src/circular-queue.h:41: class SamplingCircularQueue {
While we're already cleaning up this class, let's turn it into a template with
template parameters for the record type T and the constant number N of records,
i.e.

template <typename T, unsigned N>
class SamplingCircularQueue {
...
};

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.h#newco...
src/circular-queue.h:63: typedef AtomicWord Cell;
Get rid of this typedef, it's just confusing. Also there's no need to use
AtomicWord, and Atomic32 is sufficient.

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.h#newco...
src/circular-queue.h:88: ConsumerPosition* consumer_pos_;
Let's also cleanup this mess. Assuming that the record size and number of
records is constant (see comment about templatizing), we should be able to
replace this with:

STATIC_ASSERT(kProcessorCacheLineSize % sizeof(Atomic32) == 0);
static const unsigned kRecordStateOffset = ROUND_UP(sizeof(T),
sizeof(Atomic32));
static const unsigned kRecordSize = ROUND_UP(kRecordStateOffset +
sizeof(Atomic32), kProcessorCacheLineSize) / sizeof(Atomic32);
Atomic32 buffer_[N * kRecordSize] ALIGNED_AT(kProcessorCacheLineSize);
// Allocate enqueue_pos_ and dequeue_pos_ to different cache lines.
Atomic32 enqueue_pos_ ALIGNED_AT(kProcessorCacheLineSize);
int dequeue_pos_ ALIGNED_AT(kProcessorCacheLineSize);

where ROUND_UP(size, alignment) is a macro version of RoundUp() and
ALIGNED_AT(x) is an appropriate macro that expands to
__attribute__((aligned(x))) for GCC, Clang & Co. or __declspec(align(x)) for
MSVC.

https://codereview.chromium.org/22849002/diff/2001/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

https://codereview.chromium.org/22849002/diff/2001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:122: // feet.
We do no longer make a local copy here, so this comment is obsolete.

[Benedikt Meurer, 2013-08-13 09:37:38]

BTW I did not carefully check the memory-ordering semantics of my suggestions
above.

[yurys, 2013-08-13 10:03:43]

On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> I don't think the new implementation actually improves the situation. The case
> for multiple producers is still not handled correctly. We need to cleanup this
> whole thing. See my comments below for suggestions how to proceed. CC'ing Sven
> for additional feedback.
> 
Multiple producers is explicit non-goal for this implementation. The queue is
supposed to be used as SPSC one as there is always a dedicated processing thread
for the profiled one.


> https://codereview.chromium.org/22849002/diff/2001/src/circular-queue-inl.h
> File src/circular-queue-inl.h (right):
> 
>
https://codereview.chromium.org/22849002/diff/2001/src/circular-queue-inl.h#n...
> src/circular-queue-inl.h:50: }
> This assumes that there is always only a single producer, but this is not
always
> the case, for example in Chrome on Linux with profiled web worker threads, and
> maybe some other scenarios. We should get this right now while we're at it.
> 
Each web worker uses its own isolate and its own processing thread. The
implementation explicitly assumes that we have 1 to 1 relation between profiled
threads and processing ones so there is no need for MPSC queue. The profiler
event processing thread doesn't support events from multiple threads.

[yurys, 2013-08-13 10:05:20]

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc
File src/circular-queue.cc (right):

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc#newc...
src/circular-queue.cc:81: MemoryBarrier();
On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> Why do we need this memory barrier here?

Because we'd like to see up-to-date value at consumer_pos_->dequeue_pos and
Acquire_Load doesn't guarantee that we won't use some cached version of it.

[Benedikt Meurer, 2013-08-13 10:28:12]

On 2013/08/13 10:05:20, Yury Semikhatsky wrote:
> https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc
> File src/circular-queue.cc (right):
> 
>
https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc#newc...
> src/circular-queue.cc:81: MemoryBarrier();
> On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> > Why do we need this memory barrier here?
> 
> Because we'd like to see up-to-date value at consumer_pos_->dequeue_pos and
> Acquire_Load doesn't guarantee that we won't use some cached version of it.

Right, isn't Release_Load what you need here, assuming that you use
Acquire_Store?

[Benedikt Meurer, 2013-08-13 11:14:59]

On 2013/08/13 10:03:43, Yury Semikhatsky wrote:
> On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> > I don't think the new implementation actually improves the situation. The
case
> > for multiple producers is still not handled correctly. We need to cleanup
this
> > whole thing. See my comments below for suggestions how to proceed. CC'ing
Sven
> > for additional feedback.
> > 
> Multiple producers is explicit non-goal for this implementation. The queue is
> supposed to be used as SPSC one as there is always a dedicated processing
thread
> for the profiled one.
> 
> 
> > https://codereview.chromium.org/22849002/diff/2001/src/circular-queue-inl.h
> > File src/circular-queue-inl.h (right):
> > 
> >
>
https://codereview.chromium.org/22849002/diff/2001/src/circular-queue-inl.h#n...
> > src/circular-queue-inl.h:50: }
> > This assumes that there is always only a single producer, but this is not
> always
> > the case, for example in Chrome on Linux with profiled web worker threads,
and
> > maybe some other scenarios. We should get this right now while we're at it.
> > 
> Each web worker uses its own isolate and its own processing thread. The
> implementation explicitly assumes that we have 1 to 1 relation between
profiled
> threads and processing ones so there is no need for MPSC queue. The profiler
> event processing thread doesn't support events from multiple threads.

If there's a 1:1 relationship between profiled and profiler threads, then there
is no need for a queue at all. The profiler thread could just suspend the
profiled thread, collect the sample and resume the profiled thread. This is easy
on Mac and Windows where we have thread_suspend/ThreadSuspend. For Linux,
Solaris and *BSD we may need to use semaphores and pthread_kill().

[yurys, 2013-08-13 11:59:53]

On 2013/08/13 10:28:12, Benedikt Meurer wrote:
> On 2013/08/13 10:05:20, Yury Semikhatsky wrote:
> > https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc
> > File src/circular-queue.cc (right):
> > 
> >
>
https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc#newc...
> > src/circular-queue.cc:81: MemoryBarrier();
> > On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> > > Why do we need this memory barrier here?
> > 
> > Because we'd like to see up-to-date value at consumer_pos_->dequeue_pos and
> > Acquire_Load doesn't guarantee that we won't use some cached version of it.
> 
> Right, isn't Release_Load what you need here, assuming that you use
> Acquire_Store?

I'm confused which Acquire_Store do you mean? Actually given that we use memory
barrier here it seems to me that we don't need to use Acquire_/Release_Load here
at all.

[Benedikt Meurer, 2013-08-13 12:01:40]

On 2013/08/13 11:59:53, Yury Semikhatsky wrote:
> On 2013/08/13 10:28:12, Benedikt Meurer wrote:
> > On 2013/08/13 10:05:20, Yury Semikhatsky wrote:
> > > https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc
> > > File src/circular-queue.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc#newc...
> > > src/circular-queue.cc:81: MemoryBarrier();
> > > On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> > > > Why do we need this memory barrier here?
> > > 
> > > Because we'd like to see up-to-date value at consumer_pos_->dequeue_pos
and
> > > Acquire_Load doesn't guarantee that we won't use some cached version of
it.
> > 
> > Right, isn't Release_Load what you need here, assuming that you use
> > Acquire_Store?
> 
> I'm confused which Acquire_Store do you mean? Actually given that we use
memory
> barrier here it seems to me that we don't need to use Acquire_/Release_Load
here
> at all.

The one in FinishDequeue(), where you currently use Release_Store. But right, if
you use the explicit barrier, then you should get away with
NoBarrier_Load/Store.

[yurys, 2013-08-13 14:10:29]

Benedikt, please take another look.

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc
File src/circular-queue.cc (right):

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.cc#newc...
src/circular-queue.cc:94: }
On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> T* StartDequeue() {
>   if (Acquire_Load(&buffer_[dequeue_pos_ + kRecordStateOffset] != kFull)
return
> NULL;
>   return reinterpret_cast<T*>(&buffer_[dequeue_pos_]);
> }
> 
> void FinishDequeue() {
>   ASSERT(Release_Load(&buffer_[dequeue_pos_ + kRecordStateOffset] == kFull);
>   Release_Store(&buffer_[dequeue_pos_ + kRecordStateOffset], kEmpty);
>   dequeue_pos_ = (dequeue_pos + kRecordSize) % (N * kRecordSize);
> }
Changed that code.

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.h
File src/circular-queue.h (right):

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.h#newco...
src/circular-queue.h:41: class SamplingCircularQueue {
On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> While we're already cleaning up this class, let's turn it into a template with
> template parameters for the record type T and the constant number N of
records,
> i.e.
> 
> template <typename T, unsigned N>
> class SamplingCircularQueue {
> ...
> };

Done.

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.h#newco...
src/circular-queue.h:63: typedef AtomicWord Cell;
On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> Get rid of this typedef, it's just confusing. Also there's no need to use
> AtomicWord, and Atomic32 is sufficient.

Done.

https://codereview.chromium.org/22849002/diff/2001/src/circular-queue.h#newco...
src/circular-queue.h:88: ConsumerPosition* consumer_pos_;
On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> Let's also cleanup this mess. Assuming that the record size and number of
> records is constant (see comment about templatizing), we should be able to
> replace this with:
> 
> STATIC_ASSERT(kProcessorCacheLineSize % sizeof(Atomic32) == 0);
> static const unsigned kRecordStateOffset = ROUND_UP(sizeof(T),
> sizeof(Atomic32));
> static const unsigned kRecordSize = ROUND_UP(kRecordStateOffset +
> sizeof(Atomic32), kProcessorCacheLineSize) / sizeof(Atomic32);
> Atomic32 buffer_[N * kRecordSize] ALIGNED_AT(kProcessorCacheLineSize);
> // Allocate enqueue_pos_ and dequeue_pos_ to different cache lines.
> Atomic32 enqueue_pos_ ALIGNED_AT(kProcessorCacheLineSize);
> int dequeue_pos_ ALIGNED_AT(kProcessorCacheLineSize);
> 
> where ROUND_UP(size, alignment) is a macro version of RoundUp() and
> ALIGNED_AT(x) is an appropriate macro that expands to
> __attribute__((aligned(x))) for GCC, Clang & Co. or __declspec(align(x)) for
> MSVC.

Done.

https://codereview.chromium.org/22849002/diff/2001/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

https://codereview.chromium.org/22849002/diff/2001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:122: // feet.
On 2013/08/13 09:31:32, Benedikt Meurer wrote:
> We do no longer make a local copy here, so this comment is obsolete.

Done.

[Benedikt Meurer, 2013-08-13 14:29:41]

Ok, next round of comments.

https://codereview.chromium.org/22849002/diff/23001/src/allocation.h
File src/allocation.h (right):

https://codereview.chromium.org/22849002/diff/23001/src/allocation.h#newcode39
src/allocation.h:39: //   ALIGNAS(16) int array[4];
Typo: ALIGN_AT

https://codereview.chromium.org/22849002/diff/23001/src/allocation.h#newcode44
src/allocation.h:44: #endif
Missing #else for other compilers.

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue-inl.h
File src/circular-queue-inl.h (right):

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue-inl.h#...
src/circular-queue-inl.h:51: if (NoBarrier_Load(&dequeue_pos_->marker) == kFull)
{
Thinking about this again, we may need an Acquire_Load() to be safe in addition
to the MemoryBarrier() above.

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue-inl.h#...
src/circular-queue-inl.h:68: if (NoBarrier_Load(&enqueue_pos_->marker) ==
kEmpty) {
Also use Acquire_Load() here to be sure.

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue.h
File src/circular-queue.h (right):

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue.h#newc...
src/circular-queue.h:74: STATIC_ASSERT(kProcessorCacheLineSize == 64);
We should probably turn that kProcessorCacheLineSize into a #define, so we can
use it directly.

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue.h#newc...
src/circular-queue.h:78: Atomic32 marker ALIGN_AT(sizeof(Atomic32));
The ALIGN_AT here is redundant, as the compiler will automatically align
properly.

[yurys, 2013-08-13 14:52:37]

PTAL

https://codereview.chromium.org/22849002/diff/23001/src/allocation.h
File src/allocation.h (right):

https://codereview.chromium.org/22849002/diff/23001/src/allocation.h#newcode39
src/allocation.h:39: //   ALIGNAS(16) int array[4];
On 2013/08/13 14:29:42, Benedikt Meurer wrote:
> Typo: ALIGN_AT

Done.

https://codereview.chromium.org/22849002/diff/23001/src/allocation.h#newcode44
src/allocation.h:44: #endif
On 2013/08/13 14:29:42, Benedikt Meurer wrote:
> Missing #else for other compilers.

Done. Added #error for other compilers.

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue-inl.h
File src/circular-queue-inl.h (right):

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue-inl.h#...
src/circular-queue-inl.h:51: if (NoBarrier_Load(&dequeue_pos_->marker) == kFull)
{
On 2013/08/13 14:29:42, Benedikt Meurer wrote:
> Thinking about this again, we may need an Acquire_Load() to be safe in
addition
> to the MemoryBarrier() above.

Done.

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue-inl.h#...
src/circular-queue-inl.h:68: if (NoBarrier_Load(&enqueue_pos_->marker) ==
kEmpty) {
On 2013/08/13 14:29:42, Benedikt Meurer wrote:
> Also use Acquire_Load() here to be sure.

Done.

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue.h
File src/circular-queue.h (right):

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue.h#newc...
src/circular-queue.h:74: STATIC_ASSERT(kProcessorCacheLineSize == 64);
On 2013/08/13 14:29:42, Benedikt Meurer wrote:
> We should probably turn that kProcessorCacheLineSize into a #define, so we can
> use it directly.

Done.

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue.h#newc...
src/circular-queue.h:78: Atomic32 marker ALIGN_AT(sizeof(Atomic32));
On 2013/08/13 14:29:42, Benedikt Meurer wrote:
> The ALIGN_AT here is redundant, as the compiler will automatically align
> properly.
Done. But why are they always aligned?

[yurys, 2013-08-13 15:28:53]

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue.h
File src/circular-queue.h (right):

https://codereview.chromium.org/22849002/diff/23001/src/circular-queue.h#newc...
src/circular-queue.h:78: Atomic32 marker ALIGN_AT(sizeof(Atomic32));
On 2013/08/13 14:52:37, Yury Semikhatsky wrote:
> On 2013/08/13 14:29:42, Benedikt Meurer wrote:
> > The ALIGN_AT here is redundant, as the compiler will automatically align
> > properly.
> Done. But why are they always aligned?
Ok, I see, it should be just naturally aligned by the compiler.

[Benedikt Meurer, 2013-08-14 11:38:09]

https://codereview.chromium.org/22849002/diff/32001/src/allocation.h
File src/allocation.h (right):

https://codereview.chromium.org/22849002/diff/32001/src/allocation.h#newcode36
src/allocation.h:36: // Specify memory alignment for structs, classes, etc.
Please move the ALIGN_AT definition to globals.h, near OFFSET_OF maybe.

https://codereview.chromium.org/22849002/diff/32001/src/allocation.h#newcode42
src/allocation.h:42: #elif defined(__GNUC__) || defined(__APPLE__)
Why __APPLE__ here? This is actually a compiler thing and therefore unrelated to
the operating system (and there's only GCC and clang on OS X anyway, which both
define __GNUC__).

https://codereview.chromium.org/22849002/diff/32001/src/allocation.h#newcode45
src/allocation.h:45: #error "ALIGN_AT is not supported for your compiler"
Since the alignment is an optimization, I think it's fine to just #define
ALIGN_AT(x) in this case.

[yurys, 2013-08-14 13:19:00]

https://codereview.chromium.org/22849002/diff/32001/src/allocation.h
File src/allocation.h (right):

https://codereview.chromium.org/22849002/diff/32001/src/allocation.h#newcode36
src/allocation.h:36: // Specify memory alignment for structs, classes, etc.
On 2013/08/14 11:38:09, Benedikt Meurer wrote:
> Please move the ALIGN_AT definition to globals.h, near OFFSET_OF maybe.

Done.

https://codereview.chromium.org/22849002/diff/32001/src/allocation.h#newcode42
src/allocation.h:42: #elif defined(__GNUC__) || defined(__APPLE__)
On 2013/08/14 11:38:09, Benedikt Meurer wrote:
> Why __APPLE__ here? This is actually a compiler thing and therefore unrelated
to
> the operating system (and there's only GCC and clang on OS X anyway, which
both
> define __GNUC__).

Done. Removed __APPLE__.

https://codereview.chromium.org/22849002/diff/32001/src/allocation.h#newcode45
src/allocation.h:45: #error "ALIGN_AT is not supported for your compiler"
On 2013/08/14 11:38:09, Benedikt Meurer wrote:
> Since the alignment is an optimization, I think it's fine to just #define
> ALIGN_AT(x) in this case.

Done.

[Benedikt Meurer, 2013-08-19 06:12:10]

Ok, almost there. Just one nit with ALIGN_AT(). Please rename to ALIGNAS() and
use alignas() with C++11.

[yurys, 2013-08-19 06:30:44]

On 2013/08/19 06:12:10, Benedikt Meurer wrote:
> Ok, almost there. Just one nit with ALIGN_AT(). Please rename to ALIGNAS() and
> use alignas() with C++11.

Done.

[Benedikt Meurer, 2013-08-19 06:43:57]

On 2013/08/19 06:30:44, Yury Semikhatsky wrote:
> On 2013/08/19 06:12:10, Benedikt Meurer wrote:
> > Ok, almost there. Just one nit with ALIGN_AT(). Please rename to ALIGNAS()
and
> > use alignas() with C++11.
> 
> Done.

Just noticed, that ALIGNAS() is still in allocation.h. Please move to globals.h
also. Then LGTM. You'll probably need to wait another week to land this as we're
in "Stabilization Week, Part 2" now.

[yurys, 2013-08-19 06:58:50]

On 2013/08/19 06:43:57, Benedikt Meurer wrote:
> On 2013/08/19 06:30:44, Yury Semikhatsky wrote:
> > On 2013/08/19 06:12:10, Benedikt Meurer wrote:
> > > Ok, almost there. Just one nit with ALIGN_AT(). Please rename to ALIGNAS()
> and
> > > use alignas() with C++11.
> > 
> > Done.
> 
> Just noticed, that ALIGNAS() is still in allocation.h. Please move to
globals.h
> also. Then LGTM.
Done. Moved ALIGNAS to globals.h 


> You'll probably need to wait another week to land this as we're
> in "Stabilization Week, Part 2" now.

Sure, will wait until the tree is open. Hopefully it will be sooner than 1 more
week.

[Benedikt Meurer, 2013-08-23 07:47:32]

On 2013/08/19 06:58:50, Yury Semikhatsky wrote:
> On 2013/08/19 06:43:57, Benedikt Meurer wrote:
> > On 2013/08/19 06:30:44, Yury Semikhatsky wrote:
> > > On 2013/08/19 06:12:10, Benedikt Meurer wrote:
> > > > Ok, almost there. Just one nit with ALIGN_AT(). Please rename to
ALIGNAS()
> > and
> > > > use alignas() with C++11.
> > > 
> > > Done.
> > 
> > Just noticed, that ALIGNAS() is still in allocation.h. Please move to
> globals.h
> > also. Then LGTM.
> Done. Moved ALIGNAS to globals.h 
> 
> 
> > You'll probably need to wait another week to land this as we're
> > in "Stabilization Week, Part 2" now.
> 
> Sure, will wait until the tree is open. Hopefully it will be sooner than 1
more
> week.

Can you please rebase this on bleeding_edge and use the newly added
V8_ALIGNAS()?

[yurys, 2013-08-23 08:11:47]

On 2013/08/23 07:47:32, Benedikt Meurer wrote:
> On 2013/08/19 06:58:50, Yury Semikhatsky wrote:
> > On 2013/08/19 06:43:57, Benedikt Meurer wrote:
> > > On 2013/08/19 06:30:44, Yury Semikhatsky wrote:
> > > > On 2013/08/19 06:12:10, Benedikt Meurer wrote:
> > > > > Ok, almost there. Just one nit with ALIGN_AT(). Please rename to
> ALIGNAS()
> > > and
> > > > > use alignas() with C++11.
> > > > 
> > > > Done.
> > > 
> > > Just noticed, that ALIGNAS() is still in allocation.h. Please move to
> > globals.h
> > > also. Then LGTM.
> > Done. Moved ALIGNAS to globals.h 
> > 
> > 
> > > You'll probably need to wait another week to land this as we're
> > > in "Stabilization Week, Part 2" now.
> > 
> > Sure, will wait until the tree is open. Hopefully it will be sooner than 1
> more
> > week.
> 
> Can you please rebase this on bleeding_edge and use the newly added
> V8_ALIGNAS()?

Done.

[Benedikt Meurer, 2013-08-23 08:14:39]

LGTM again, please go ahead.

[yurys, 2013-08-23 08:22:26]

Committed patchset #12 manually as r16284.