Merge google-perftools r109 (the current contents of third_party/tcmalloc/vendor)

Merge google-perftools r109 (the current contents of third_party/tcmalloc/vendor)
with the forked Chromium version of tcmalloc.

This change also requires some fixes to base/allocator/allocator.gyp: new tcmalloc
source files added, unittest_utils.cc disabled (tcmalloc has its own snprintf()
implementation now)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89452

[Alexander Potapenko, 2011-06-01 13:33:08]

Folks,

could you please review this?
Please pay additional attention to the files with non-empty "Delta from patch
set" column.

[antonm, 2011-06-01 14:01:00]

Alexander,

may you provide us with a list of files which required not trivial merge?  It's
hard to review many files, most of those are just added or were trivially
merged.

yours,
anton.

[Alexander Potapenko, 2011-06-01 14:35:30]

Sure, here it is:

http://codereview.chromium.org/7050034/diff/22146/base/allocator/allocator.gyp
http://codereview.chromium.org/7050034/diff/22146/base/allocator/allocator_sh...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/README...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...

Sorry for not putting this list down initially.

I didn't have an opportunity to re-generate the configs for Windows and FreeBSD,
so those may possibly be incorrect.

[Alexander Potapenko, 2011-06-01 14:45:59]

Also some Windows-related fixes:
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
-- I had to introduce the HAVE_SYS_TIME_H macro, which is not in the original
perftools yet, and update the config*.h files accordingly.
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
,
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
,
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
,
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
-- added the "#ifdef HAVE_STDINT_H"

[antonm, 2011-06-02 13:37:35]

First round of comments.

I didn't spot something which looked like merge conflicts in three files (see
comments below) and stopped to look into stuff which is apparently wasn't
tweaked for Chromium.  So the following files were reviewed:

http://codereview.chromium.org/7050034/diff/22146/base/allocator/allocator.gyp
http://codereview.chromium.org/7050034/diff/22146/base/allocator/allocator_sh...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/README...

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...

And all from the second email.

Sorry for asking you to provide more info.  Merges are hard to review as you
never know which parts should be reviewed.

http://codereview.chromium.org/7050034/diff/22146/base/allocator/allocator_sh...
File base/allocator/allocator_shim.cc (right):

http://codereview.chromium.org/7050034/diff/22146/base/allocator/allocator_sh...
base/allocator/allocator_shim.cc:8: #include "base/sysinfo.h"
I hope it's not an accidental edit.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h:31: * This file is a
Linux-specific part of spinlock_internal.cc
which parts were conflicts on merge?

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/config_freebsd.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/config_freebsd.h:76: /* Define to 1 if you
have the <malloc/malloc.h> header file. */
which parts didn't merge automatically?

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/config_linux.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/config_linux.h:11: #undef HAVE_CYGWIN_SIGNAL_H
ditto

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/page_heap.cc:35: #include <inttypes.h>        
          // for PRIuPTR
I'd suggest to unify indenting: either fixed one or to space after the end of
statement or all starting at the same column.  WDYT?

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/page_heap.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/page_heap.h:206: static const int
kMinSystemAlloc = kMaxPages;
where kMaxPages are defined now?

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/windows/port.cc (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/windows/port.cc:197: // It is on x86 and x64,
where Windows runs.
may you elaborate why?  e.g. does it require pthread_once_t to be volatile or
something like that?

Or it was just brought with the merge?

[Alexander Potapenko, 2011-06-09 09:35:24]

I've marked the places where code differs from the upstream version. The problem
with config_{windows,freebsd}.h is that I didn't generate them automatically, so
I needed a fresh eye on them. config_linux.h was basically merged with an
autogenerated file (still not any of the upstream files)

I've also synced the CL with the Chromium head, so ptrdiff_t occurrences got
changed into std::ptrdiff_t

http://codereview.chromium.org/7050034/diff/22146/base/allocator/allocator_sh...
File base/allocator/allocator_shim.cc (right):

http://codereview.chromium.org/7050034/diff/22146/base/allocator/allocator_sh...
base/allocator/allocator_shim.cc:8: #include "base/sysinfo.h"
On 2011/06/02 13:37:35, antonm wrote:
> I hope it's not an accidental edit.

I've checked: without this line liballocator fails to compile.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/atomicops.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/atomicops.h:99: #include
"base/atomicops-internals-windows.h"
This was a fix rather than a merge:
http://code.google.com/p/google-perftools/source/browse/trunk/src/base/atomic...

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h:31: * This file is a
Linux-specific part of spinlock_internal.cc
On 2011/06/02 13:37:35, antonm wrote:
> which parts were conflicts on merge?
Well, these weren't conflicts, but some lines had to be changed manually,
because the upstream tcmalloc doesn't use syscalls. Just wanted them to be
checked.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h:96: FUTEX_WAIT |
futex_private_flag,
Here.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/config_freebsd.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/config_freebsd.h:76: /* Define to 1 if you
have the <malloc/malloc.h> header file. */
On 2011/06/02 13:37:35, antonm wrote:
> which parts didn't merge automatically?

This file is not in the perftools repo at all, so I've added the necessary
definitions such that they make sense. I didn't test the BSD build, however.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/config_linux.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/config_linux.h:11: #undef HAVE_CYGWIN_SIGNAL_H
On 2011/06/02 13:37:35, antonm wrote:
> ditto

This file was merged with a config.h I got from running ./configure on my
desktop. This is not an upstream merge technically

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/config_win.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/config_win.h:41: #undef HAVE_CYGWIN_SIGNAL_H
This config was fixed manually, too.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/page_heap.cc:35: #include <inttypes.h>        
          // for PRIuPTR
On 2011/06/02 13:37:35, antonm wrote:
> I'd suggest to unify indenting: either fixed one or to space after the end of
> statement or all starting at the same column.  WDYT?

I think this line was merged automatically:
http://code.google.com/p/google-perftools/source/browse/trunk/src/page_heap.cc
I'd prefer not to make minor style changes that will conflict with the upstream
version.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/page_heap.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/page_heap.h:206: static const int
kMinSystemAlloc = kMaxPages;
On 2011/06/02 13:37:35, antonm wrote:
> where kMaxPages are defined now?

In third_party/tcmalloc/chromium/src/common.h

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/windows/port.cc (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/windows/port.cc:197: // It is on x86 and x64,
where Windows runs.
On 2011/06/02 13:37:35, antonm wrote:
> may you elaborate why?  e.g. does it require pthread_once_t to be volatile or
> something like that?
> 
> Or it was just brought with the merge?

The latter one,
http://code.google.com/p/google-perftools/source/browse/trunk/src/windows/por...

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/windows/port.cc:222: SysAllocator* sys_alloc =
NULL;
These lines differ from the upstream version: we need to declare sys_alloc
somewhere on Windows.

[antonm, 2011-06-14 14:15:35]

LGTM for the files.  Thanks a lot for keeping us up with perftools.

BTW, do you know how far away are we?  And is it feasible to end the fork?

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/config_win.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/config_win.h:80: #undef HAVE_FEATURES_H
it looks like you default to #undef all the features, is it the best approach?

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/windows/port.cc (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/windows/port.cc:222: SysAllocator* sys_alloc =
NULL;
On 2011/06/09 09:35:24, Alexander Potapenko wrote:
> These lines differ from the upstream version: we need to declare sys_alloc
> somewhere on Windows.

Please, add a comment and consider backporting this to perftools.

[Alexander Potapenko, 2011-06-15 16:32:20]

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/config_win.h (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/config_win.h:80: #undef HAVE_FEATURES_H
On 2011/06/14 14:15:35, antonm wrote:
> it looks like you default to #undef all the features, is it the best approach?
This guarantees we don't break anything while upgrading:
because autoconf is not actually used to generate the configs, it's not possible
that someone else defines some HAVE_FOOBAR_H macro and we undefine it. If
defining some feature makes the code more optimal (which is unlikely for
perftools), it can then be enabled in a separate CL.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/windows/port.cc (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/windows/port.cc:222: SysAllocator* sys_alloc =
NULL;
On 2011/06/14 14:15:35, antonm wrote:
> On 2011/06/09 09:35:24, Alexander Potapenko wrote:
> > These lines differ from the upstream version: we need to declare sys_alloc
> > somewhere on Windows.
> 
> Please, add a comment and consider backporting this to perftools.

For some strange reason the trunk perftools do not need this line.
Investigating.

[Alexander Potapenko, 2011-06-16 14:00:46]

> BTW, do you know how far away are we?  And is it feasible to end the fork?

Do you mean abandoning the forked directory at all?
The merge of tcmalloc.cc still requires some human intervention, so I doubt we
can do that. The forked version is also tuned fine to commit the memory
aggressively, which is not necessary in the upstream version.

[Alexander Potapenko, 2011-06-16 14:49:47]

Will, Mike,
do you object against submitting this CL?
I was going to do that tomorrow (MSD) in order to have some time to monitor the
perf bots.

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/windows/port.cc (right):

http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/windows/port.cc:222: SysAllocator* sys_alloc =
NULL;
On 2011/06/14 14:15:35, antonm wrote:
> On 2011/06/09 09:35:24, Alexander Potapenko wrote:
> > These lines differ from the upstream version: we need to declare sys_alloc
> > somewhere on Windows.
> 
> Please, add a comment and consider backporting this to perftools.

Done.
I've also put system-alloc.cc back to the list of files excluded on Windows in
base/allocator/allocator.gyp

[willchan no longer on Chromium, 2011-06-16 15:00:14]

I haven't looked, but I'm happy with Anton's review. Go for it! Thanks
a lot for doing this.

On Thu, Jun 16, 2011 at 5:49 PM,  <glider@chromium.org> wrote:
> Will, Mike,
> do you object against submitting this CL?
> I was going to do that tomorrow (MSD) in order to have some time to monitor
> the
> perf bots.
>
>
>
>
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
> File third_party/tcmalloc/chromium/src/windows/port.cc (right):
>
>
http://codereview.chromium.org/7050034/diff/22146/third_party/tcmalloc/chromi...
> third_party/tcmalloc/chromium/src/windows/port.cc:222: SysAllocator*
> sys_alloc = NULL;
> On 2011/06/14 14:15:35, antonm wrote:
>>
>> On 2011/06/09 09:35:24, Alexander Potapenko wrote:
>> > These lines differ from the upstream version: we need to declare
>
> sys_alloc
>>
>> > somewhere on Windows.
>
>> Please, add a comment and consider backporting this to perftools.
>
> Done.
> I've also put system-alloc.cc back to the list of files excluded on
> Windows in base/allocator/allocator.gyp
>
> http://codereview.chromium.org/7050034/
>

[antonm, 2011-06-16 15:44:42]

Yep.  Maybe we can put the differences under compile time flag or something.

yours,
anton.

On Thu, Jun 16, 2011 at 6:00 PM,  <glider@chromium.org> wrote:
>> BTW, do you know how far away are we?  And is it feasible to end the fork?
>
> Do you mean abandoning the forked directory at all?
> The merge of tcmalloc.cc still requires some human intervention, so I doubt
> we
> can do that. The forked version is also tuned fine to commit the memory
> aggressively, which is not necessary in the upstream version.
>
> http://codereview.chromium.org/7050034/
>