Fixed alignment issues of ProfilerEventsProcessor.

Fixed alignment issues of ProfilerEventsProcessor.

R=bmeurer@chromium.org

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

[Benedikt Meurer, 2014-01-09 10:42:52]

First round of comments.

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc
File src/allocation.cc (right):

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode35
src/allocation.cc:35: #if V8_CC_MSVC
What about Cygwin? Does Cygwin have posix_memalign? What about mingw?

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode37
src/allocation.cc:37: #define V8_HAS__ALIGNED_MALLOC 1
Shouldn't these V8_HAS_* macros live in v8config.h?

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode41
src/allocation.cc:41: #include <stdlib.h>  // NOLINT
stdlib.h is already included above.

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode114
src/allocation.cc:114: #if !V8_HAS__ALIGNED_MALLOC && !V8_HAS_POSIX_MEMALIGN
This looks weird. Can't we inline these trivial functions below?

https://codereview.chromium.org/131393002/diff/1/src/allocation.h
File src/allocation.h (right):

https://codereview.chromium.org/131393002/diff/1/src/allocation.h#newcode112
src/allocation.h:112: void* AlignedMalloc(size_t size, size_t alignment);
Nit: Using AlignedNew and AlignedDelete sounds more consistent.

[Sven Panne, 2014-01-09 12:04:19]

New version with Bionic fixes + incorporated feedback. Furthermore,
AlignedMalloc has been renamed to AlignedAlloc, following the C11 naming more
closely. PTAL again...

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc
File src/allocation.cc (right):

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode35
src/allocation.cc:35: #if V8_CC_MSVC
On 2014/01/09 10:42:52, Benedikt Meurer wrote:
> What about Cygwin? Does Cygwin have posix_memalign? What about mingw?

Cygwin seems to have posix_memalign, and hopefully MinGW exposes the
_aligned_malloc stuff (slightly contradicting articles found on the net). I'll
change the test here to V8_OS_WIN, because IIRC a normal Cygwin build (without
-mno-cygwin) does not #define _WIN32. But let the bots decide. :-P

We really, really need to pull in Chrome's base library...

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode37
src/allocation.cc:37: #define V8_HAS__ALIGNED_MALLOC 1
On 2014/01/09 10:42:52, Benedikt Meurer wrote:
> Shouldn't these V8_HAS_* macros live in v8config.h?

I don't have a strong opinion about that, up to now there are no library-related
#defines there AFACIT. Will move them to that header...

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode41
src/allocation.cc:41: #include <stdlib.h>  // NOLINT
On 2014/01/09 10:42:52, Benedikt Meurer wrote:
> stdlib.h is already included above.

Done.

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode114
src/allocation.cc:114: #if !V8_HAS__ALIGNED_MALLOC && !V8_HAS_POSIX_MEMALIGN
On 2014/01/09 10:42:52, Benedikt Meurer wrote:
> This looks weird. Can't we inline these trivial functions below?

I explicitly outlined them, and I actually think it makes things more readable
and follows the DRY principle. Otherwise it is a total casting & bit fiddling
mess. Well it already is, but... ;-) I would prefer to leave the helpers.

[Sven Panne, 2014-01-09 12:04:19]

New version with Bionic fixes + incorporated feedback. Furthermore,
AlignedMalloc has been renamed to AlignedAlloc, following the C11 naming more
closely. PTAL again...

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc
File src/allocation.cc (right):

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode35
src/allocation.cc:35: #if V8_CC_MSVC
On 2014/01/09 10:42:52, Benedikt Meurer wrote:
> What about Cygwin? Does Cygwin have posix_memalign? What about mingw?

Cygwin seems to have posix_memalign, and hopefully MinGW exposes the
_aligned_malloc stuff (slightly contradicting articles found on the net). I'll
change the test here to V8_OS_WIN, because IIRC a normal Cygwin build (without
-mno-cygwin) does not #define _WIN32. But let the bots decide. :-P

We really, really need to pull in Chrome's base library...

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode37
src/allocation.cc:37: #define V8_HAS__ALIGNED_MALLOC 1
On 2014/01/09 10:42:52, Benedikt Meurer wrote:
> Shouldn't these V8_HAS_* macros live in v8config.h?

I don't have a strong opinion about that, up to now there are no library-related
#defines there AFACIT. Will move them to that header...

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode41
src/allocation.cc:41: #include <stdlib.h>  // NOLINT
On 2014/01/09 10:42:52, Benedikt Meurer wrote:
> stdlib.h is already included above.

Done.

https://codereview.chromium.org/131393002/diff/1/src/allocation.cc#newcode114
src/allocation.cc:114: #if !V8_HAS__ALIGNED_MALLOC && !V8_HAS_POSIX_MEMALIGN
On 2014/01/09 10:42:52, Benedikt Meurer wrote:
> This looks weird. Can't we inline these trivial functions below?

I explicitly outlined them, and I actually think it makes things more readable
and follows the DRY principle. Otherwise it is a total casting & bit fiddling
mess. Well it already is, but... ;-) I would prefer to leave the helpers.

[Benedikt Meurer, 2014-01-10 09:31:19]

As discussed offline: Just check for V8_OS_WIN32 and use _aligned_malloc(),
otherwise if V8_LIBC_BIONIC then include <malloc.h> and use memalign(), fallback
(without check) is posix_memalign().

[Benedikt Meurer, 2014-01-10 09:55:17]

LGTM

[Sven Panne, 2014-01-10 10:39:56]

Committed patchset #5 manually as r18526 (presubmit successful).