Merged third_party/tcmalloc/vendor/src(google-perftools r87) into...

Merged third_party/tcmalloc/vendor/src(google-perftools r87) into
third_party/tcmalloc/chromium/src.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41938

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41950

[willchan no longer on Chromium, 2010-02-04 19:48:47]

It looks like it fails on the windows trybot.  Are you doing something about
that?

[Alexander Potapenko, 2010-02-04 20:06:29]

On 2010/02/04 19:48:47, willchan wrote:
> It looks like it fails on the windows trybot.  Are you doing something about
> that?

I am. There are some issues with including tcmalloc.cc. As far as I understand,
on Linux liballocator should contain tcmalloc.o compiled from tcmalloc.cc, where
tcmalloc.cc includes chromium/src/google/tcmalloc.h. OTOH, on Windows we should
include tcmalloc.cc from allocator_shim.cc rather than compile tcmalloc.cc
separately. tcmalloc.cc for Windows should include
chromium/src/windows/google/tcmalloc.h, which we haven't used before. I hope to
fix this tomorrow.

[willchan no longer on Chromium, 2010-02-04 20:10:17]

+mbelshe

On Thu, Feb 4, 2010 at 12:06 PM,  <glider@chromium.org> wrote:
> Reviewers: jar, sgk, willchan, antonm,
>
> Message:
> On 2010/02/04 19:48:47, willchan wrote:
>>
>> It looks like it fails on the windows trybot.  Are you doing something
>> about
>> that?
>
> I am. There are some issues with including tcmalloc.cc. As far as I
> understand,
> on Linux liballocator should contain tcmalloc.o compiled from tcmalloc.cc,
> where
> tcmalloc.cc includes chromium/src/google/tcmalloc.h. OTOH, on Windows we
> should
> include tcmalloc.cc from allocator_shim.cc rather than compile tcmalloc.cc
> separately. tcmalloc.cc for Windows should include
> chromium/src/windows/google/tcmalloc.h, which we haven't used before. I hope
> to
> fix this tomorrow.

I haven't taken a look at this.  I'm adding Mike to comment.  If you
need help, let me know.  Otherwise, I'll take a look when you've
finished working through these issues.

>
> Description:
> Merged third_party/tcmalloc/vendor/src(google-perftools r87) into
> third_party/tcmalloc/chromium/src.
>
>
> Please review this at http://codereview.chromium.org/576001
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     base/allocator/allocator.gyp
>  M     third_party/tcmalloc/chromium/src/base/basictypes.h
>  M     third_party/tcmalloc/chromium/src/base/dynamic_annotations.h
>  M     third_party/tcmalloc/chromium/src/base/logging.h
>  M     third_party/tcmalloc/chromium/src/base/low_level_alloc.h
>  M     third_party/tcmalloc/chromium/src/base/low_level_alloc.cc
>  M     third_party/tcmalloc/chromium/src/base/spinlock.h
>  M     third_party/tcmalloc/chromium/src/base/sysinfo.h
>  M     third_party/tcmalloc/chromium/src/base/sysinfo.cc
>  M     third_party/tcmalloc/chromium/src/config.h.in
>  M     third_party/tcmalloc/chromium/src/debugallocation.cc
>  M     third_party/tcmalloc/chromium/src/google/heap-checker.h
>  M     third_party/tcmalloc/chromium/src/google/malloc_extension.h
>  M     third_party/tcmalloc/chromium/src/google/malloc_extension_c.h
>  M     third_party/tcmalloc/chromium/src/google/malloc_hook.h
>  M     third_party/tcmalloc/chromium/src/google/tcmalloc.h
>  M     third_party/tcmalloc/chromium/src/google/tcmalloc.h.in
>  M     third_party/tcmalloc/chromium/src/heap-checker.cc
>  M     third_party/tcmalloc/chromium/src/heap-profile-table.h
>  M     third_party/tcmalloc/chromium/src/heap-profile-table.cc
>  M     third_party/tcmalloc/chromium/src/malloc_extension.cc
>  M     third_party/tcmalloc/chromium/src/malloc_hook.cc
>  M     third_party/tcmalloc/chromium/src/memfs_malloc.cc
>  M     third_party/tcmalloc/chromium/src/memory_region_map.h
>  M     third_party/tcmalloc/chromium/src/packed-cache-inl.h
>  M     third_party/tcmalloc/chromium/src/page_heap.h
>  M     third_party/tcmalloc/chromium/src/page_heap.cc
>  M     third_party/tcmalloc/chromium/src/pagemap.h
>  M     third_party/tcmalloc/chromium/src/pprof
>  M     third_party/tcmalloc/chromium/src/profile-handler.cc
>  M     third_party/tcmalloc/chromium/src/profiledata.h
>  A     third_party/tcmalloc/chromium/src/solaris/libstdc++.la
>  M     third_party/tcmalloc/chromium/src/stacktrace_config.h
>  M     third_party/tcmalloc/chromium/src/stacktrace_x86-inl.h
>  M     third_party/tcmalloc/chromium/src/symbolize.h
>  M     third_party/tcmalloc/chromium/src/symbolize.cc
>  M     third_party/tcmalloc/chromium/src/tcmalloc.cc
>  M     third_party/tcmalloc/chromium/src/tests/atomicops_unittest.cc
>  M     third_party/tcmalloc/chromium/src/tests/debugallocation_test.cc
>  M     third_party/tcmalloc/chromium/src/tests/malloc_extension_c_test.c
>  M     third_party/tcmalloc/chromium/src/tests/malloc_extension_test.cc
>  M     third_party/tcmalloc/chromium/src/tests/pagemap_unittest.cc
>  M     third_party/tcmalloc/chromium/src/tests/profile-handler_unittest.cc
>  M     third_party/tcmalloc/chromium/src/tests/profiler_unittest.sh
>  M     third_party/tcmalloc/chromium/src/tests/sampling_test.sh
>  M     third_party/tcmalloc/chromium/src/tests/tcmalloc_unittest.cc
>  M     third_party/tcmalloc/chromium/src/thread_cache.cc
>  M     third_party/tcmalloc/chromium/src/windows/config.h
>  M     third_party/tcmalloc/chromium/src/windows/google/tcmalloc.h
>  M     third_party/tcmalloc/chromium/src/windows/patch_functions.cc
>  M     third_party/tcmalloc/chromium/src/windows/preamble_patcher.cc
>
>
>

[Alexander Potapenko, 2010-02-04 20:28:32]

Yes, thanks.
Sorry for adding you too early.
On 2010/02/04 20:10:17, willchan wrote:
> +mbelshe
> 
> On Thu, Feb 4, 2010 at 12:06 PM,  <mailto:glider@chromium.org> wrote:
> > Reviewers: jar, sgk, willchan, antonm,
> >
> > Message:
> > On 2010/02/04 19:48:47, willchan wrote:
> >>
> >> It looks like it fails on the windows trybot. &nbsp;Are you doing something
> >> about
> >> that?
> >
> > I am. There are some issues with including tcmalloc.cc. As far as I
> > understand,
> > on Linux liballocator should contain tcmalloc.o compiled from tcmalloc.cc,
> > where
> > tcmalloc.cc includes chromium/src/google/tcmalloc.h. OTOH, on Windows we
> > should
> > include tcmalloc.cc from allocator_shim.cc rather than compile tcmalloc.cc
> > separately. tcmalloc.cc for Windows should include
> > chromium/src/windows/google/tcmalloc.h, which we haven't used before. I hope
> > to
> > fix this tomorrow.
> 
> I haven't taken a look at this.  I'm adding Mike to comment.  If you
> need help, let me know.  Otherwise, I'll take a look when you've
> finished working through these issues.
> 
> >
> > Description:
> > Merged third_party/tcmalloc/vendor/src(google-perftools r87) into
> > third_party/tcmalloc/chromium/src.
> >
> >
> > Please review this at http://codereview.chromium.org/576001
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src/
> >
> > Affected files:
> > &nbsp;M &nbsp; &nbsp; base/allocator/allocator.gyp
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/base/basictypes.h
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/base/dynamic_annotations.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/base/logging.h
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/base/low_level_alloc.h
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/base/low_level_alloc.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/base/spinlock.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/base/sysinfo.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/base/sysinfo.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/config.h.in
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/debugallocation.cc
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/google/heap-checker.h
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/google/malloc_extension.h
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/google/malloc_extension_c.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/google/malloc_hook.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/google/tcmalloc.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/google/tcmalloc.h.in
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/heap-checker.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/heap-profile-table.h
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/heap-profile-table.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/malloc_extension.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/malloc_hook.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/memfs_malloc.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/memory_region_map.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/packed-cache-inl.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/page_heap.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/page_heap.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/pagemap.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/pprof
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/profile-handler.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/profiledata.h
> > &nbsp;A &nbsp; &nbsp; third_party/tcmalloc/chromium/src/solaris/libstdc++.la
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/stacktrace_config.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/stacktrace_x86-inl.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/symbolize.h
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/symbolize.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/tcmalloc.cc
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/atomicops_unittest.cc
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/debugallocation_test.cc
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/malloc_extension_c_test.c
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/malloc_extension_test.cc
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/pagemap_unittest.cc
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/profile-handler_unittest.cc
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/profiler_unittest.sh
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/sampling_test.sh
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/tests/tcmalloc_unittest.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/thread_cache.cc
> > &nbsp;M &nbsp; &nbsp; third_party/tcmalloc/chromium/src/windows/config.h
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/windows/google/tcmalloc.h
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/windows/patch_functions.cc
> > &nbsp;M &nbsp; &nbsp;
third_party/tcmalloc/chromium/src/windows/preamble_patcher.cc
> >
> >
> >
>

[sgk, 2010-02-04 23:50:55]

Based on my understanding of the intent, it looks like the forked copies of
various files go away and the .gyp configuration can be simplified.

http://codereview.chromium.org/576001/diff/5101/4052
File base/allocator/allocator.gyp (right):

http://codereview.chromium.org/576001/diff/5101/4052#newcode190
base/allocator/allocator.gyp:190: '<(tcmalloc_dir)/src/symbolize_linux.cc',
Just delete this line (and the comment).  *_linux.cc lines will get stripped out
on Windows automatically.

http://codereview.chromium.org/576001/diff/5101/4052#newcode332
base/allocator/allocator.gyp:332: '<(tcmalloc_dir)/src/page_heap_linux.cc',
IIUC, this line is obsolete and we can just get rid of it, too.  There's no
longer any use of page_heap_linux.cc anywhere in any sources list that I can
see...

http://codereview.chromium.org/576001/diff/5101/4052#newcode341
base/allocator/allocator.gyp:341: '<(tcmalloc_dir)/src/tcmalloc.cc',
I think this whole 'sources' block with its TODO(willchan) comment can also go
away.  Both of these files are now just part of the normal 'sources' list even
on Linux.

[Alexander Potapenko, 2010-02-05 08:36:57]

That's right. The first patch sets didn't contain the *_linux files, but I've
reverted that in order to fix Windows build. Now I know (probably) where the
problem is, so I'm going to delete the *_linux files.
On 2010/02/04 23:50:55, sgk wrote:
> Based on my understanding of the intent, it looks like the forked copies of
> various files go away and the .gyp configuration can be simplified.
> 
> http://codereview.chromium.org/576001/diff/5101/4052
> File base/allocator/allocator.gyp (right):
> 
> http://codereview.chromium.org/576001/diff/5101/4052#newcode190
> base/allocator/allocator.gyp:190: '<(tcmalloc_dir)/src/symbolize_linux.cc',
> Just delete this line (and the comment).  *_linux.cc lines will get stripped
out
> on Windows automatically.
> 
> http://codereview.chromium.org/576001/diff/5101/4052#newcode332
> base/allocator/allocator.gyp:332: '<(tcmalloc_dir)/src/page_heap_linux.cc',
> IIUC, this line is obsolete and we can just get rid of it, too.  There's no
> longer any use of page_heap_linux.cc anywhere in any sources list that I can
> see...
> 
> http://codereview.chromium.org/576001/diff/5101/4052#newcode341
> base/allocator/allocator.gyp:341: '<(tcmalloc_dir)/src/tcmalloc.cc',
> I think this whole 'sources' block with its TODO(willchan) comment can also go
> away.  Both of these files are now just part of the normal 'sources' list even
> on Linux.

[Alexander Potapenko, 2010-02-05 13:11:52]

Gentlemen,

please take a look at the CL (it's finally able to compile on Windows).
Have I missed any change to the forked files?

[Alexander Potapenko, 2010-02-09 10:22:59]

A friendly ping.
On 2010/02/05 13:11:52, Alexander Potapenko wrote:
> Gentlemen,
> 
> please take a look at the CL (it's finally able to compile on Windows).
> Have I missed any change to the forked files?

[Anton Muhin, 2010-02-09 13:53:43]

General questions:

1) why you undid a lot of work done on Chromium's tcmalloc?
2) did you estimate performance impact of new version?

http://codereview.chromium.org/576001/diff/3071/401
File third_party/tcmalloc/chromium/src/page_heap.cc (left):

http://codereview.chromium.org/576001/diff/3071/401#oldcode220
third_party/tcmalloc/chromium/src/page_heap.cc:220: // or after we coalesce
them.  The current code always decomits. This is
and why these changes are undone?  did you just copy over?

Please, note, that we might want to undo them given new release mechanism, but
I'd like to have it discussed first.

http://codereview.chromium.org/576001/diff/3071/401#oldcode447
third_party/tcmalloc/chromium/src/page_heap.cc:447: committed_bytes_ += (ask <<
kPageShift);
and why this is removed

[Alexander Potapenko, 2010-02-09 14:29:56]

On 2010/02/09 13:53:43, antonm1 wrote:
> General questions:
> 
> 1) why you undid a lot of work done on Chromium's tcmalloc?
In fact I was quite sure that everything was committed to perftools in parallel.
Let me check the revision history of tcmalloc/chromium/src again then.
> 2) did you estimate performance impact of new version?
Nope. I'd appreciate any info on how to do this.
> 
> http://codereview.chromium.org/576001/diff/3071/401
> File third_party/tcmalloc/chromium/src/page_heap.cc (left):
> 
> http://codereview.chromium.org/576001/diff/3071/401#oldcode220
> third_party/tcmalloc/chromium/src/page_heap.cc:220: // or after we coalesce
> them.  The current code always decomits. This is
> and why these changes are undone?  did you just copy over?
> 
> Please, note, that we might want to undo them given new release mechanism, but
> I'd like to have it discussed first.
> 
> http://codereview.chromium.org/576001/diff/3071/401#oldcode447
> third_party/tcmalloc/chromium/src/page_heap.cc:447: committed_bytes_ += (ask
<<
> kPageShift);
> and why this is removed

I'll merge back the changes done to page_heap.cc, thanks for paying attention.

[Anton Muhin, 2010-02-09 14:37:33]

On Tue, Feb 9, 2010 at 5:29 PM,  <glider@chromium.org> wrote:
> On 2010/02/09 13:53:43, antonm1 wrote:
>>
>> General questions:
>
>> 1) why you undid a lot of work done on Chromium's tcmalloc?
>
> In fact I was quite sure that everything was committed to perftools in
> parallel.
> Let me check the revision history of tcmalloc/chromium/src again then.

Thanks a lot.  And no, not everything went back into perf_tools, alas,
that's exactly why we have vendor.

>>
>> 2) did you estimate performance impact of new version?
>
> Nope. I'd appreciate any info on how to do this.

For me the most crucial are three benchmarks:

http://dromaeo.com/?dom
http://dromaeo.com/?jslib
and DOM benchmark (please, check it out from
svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would be
any troubles with access, I could just share it with you).

Ideally there should be no regressions.  Total scores are most
important, but ideally we don't regress on any (most) of the tests.

Good luck with merge and thanks a lot for doing this.

yours,
anton.

>> http://codereview.chromium.org/576001/diff/3071/401
>> File third_party/tcmalloc/chromium/src/page_heap.cc (left):
>
>> http://codereview.chromium.org/576001/diff/3071/401#oldcode220
>> third_party/tcmalloc/chromium/src/page_heap.cc:220: // or after we
>> coalesce
>> them.  The current code always decomits. This is
>> and why these changes are undone?  did you just copy over?
>
>> Please, note, that we might want to undo them given new release mechanism,
>> but
>> I'd like to have it discussed first.
>
>> http://codereview.chromium.org/576001/diff/3071/401#oldcode447
>> third_party/tcmalloc/chromium/src/page_heap.cc:447: committed_bytes_ +=
>> (ask
>
> <<
>>
>> kPageShift);
>> and why this is removed
>
> I'll merge back the changes done to page_heap.cc, thanks for paying
> attention.
>
> http://codereview.chromium.org/576001
>

[sgk, 2010-02-09 18:33:44]

gyp changes lgtm

[jar (doing other things), 2010-02-09 19:30:55]

There is a lot being dropped by this merge.  Please look at the items below, and
be sure we preserve the functionality.  This should especially include:

a) handling of committed vs decommitted spans (and special windows calls for
such support)
b) support for the on-demand memory memory, rather than just the current radix
trees (currently only supported in 32-bit windows, but probably should be ported
to 32-bit linux).
c) careful discussion and tuning of the reclamation strategy, which is very
different on client than a server.

http://codereview.chromium.org/576001/diff/3071/401
File third_party/tcmalloc/chromium/src/page_heap.cc (left):

http://codereview.chromium.org/576001/diff/3071/401#oldcode154
third_party/tcmalloc/chromium/src/page_heap.cc:154: void
PageHeap::CommitSpan(Span* span) {
As I mentioned in the header... I'm pretty sure these functions are needed.

http://codereview.chromium.org/576001/diff/3071/401#oldcode195
third_party/tcmalloc/chromium/src/page_heap.cc:195: CommitSpan(span);
This call is probably needed.

http://codereview.chromium.org/576001/diff/3071/401#oldcode197
third_party/tcmalloc/chromium/src/page_heap.cc:197: ASSERT(span->location ==
Span::IN_USE);
I think these asserts are valuable, given the extra handling of committed and
non-committed spans in the code.

http://codereview.chromium.org/576001/diff/3071/401#oldcode230
third_party/tcmalloc/chromium/src/page_heap.cc:230: // based on memory usage and
free heap sizes.
+1 for antonm's comment above.
This handling of committed and decommitted memmory, and theses comments, needs
to be preserved.

http://codereview.chromium.org/576001/diff/3071/401#oldcode239
third_party/tcmalloc/chromium/src/page_heap.cc:239: if (prev->location ==
Span::ON_RETURNED_FREELIST) {
Please retain this code (and line 235)

http://codereview.chromium.org/576001/diff/3071/401
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/3071/401#newcode272
third_party/tcmalloc/chromium/src/page_heap.cc:272: const double mult = 1000.0 /
rate;
As we move to a new reclamation approach, be very careful to tune for our client
environment.  Unlike servers, which have a fairly uniform task and memory load,
browsers have rapidly varying load, and hence need to be more responsive (and
can't assume a nice smooth asymptote).

http://codereview.chromium.org/576001/diff/3071/392
File third_party/tcmalloc/chromium/src/page_heap.h (left):

http://codereview.chromium.org/576001/diff/3071/392#oldcode74
third_party/tcmalloc/chromium/src/page_heap.h:74: typedef
TCMalloc_PageMap1_LazyCommit<32-kPageShift> Type;
This code should not be discarded.  If anything, we need to push it back
upstream.

Google3 probably doesn't care much about 32 bit systems, but for us, this is a
big deal.  The savings in meta-data allocation should be several megs, which can
add up when there are multiple processes running on a desktop.

http://codereview.chromium.org/576001/diff/3071/392#oldcode231
third_party/tcmalloc/chromium/src/page_heap.h:231: void CommitSpan(Span* span);
Windows needs actual commits, while linux seems do do such automatically (on
demand).  I suspect you need to keep thees calls to commit and decommitSpan
around... but I need to look at the rest of this CL to be sure.

http://codereview.chromium.org/576001/diff/3071/387
File third_party/tcmalloc/chromium/src/tcmalloc.cc (left):

http://codereview.chromium.org/576001/diff/3071/387#oldcode428
third_party/tcmalloc/chromium/src/tcmalloc.cc:428: stats.committed_bytes /
static_cast<double>(bytes_in_use));
I think all these stats were intentionally added... as the relate to committed
and decommitted issues.  Please preserve them.

http://codereview.chromium.org/576001/diff/3071/387#oldcode456
third_party/tcmalloc/chromium/src/tcmalloc.cc:456: "MALLOC: %12" PRIu64 " (%7.1f
MB) Bytes committed\n"
Please preserve.

http://codereview.chromium.org/576001/diff/3071/387#oldcode467
third_party/tcmalloc/chromium/src/tcmalloc.cc:467: stats.committed_bytes,
stats.committed_bytes / MB,
This is the data source to preserve.

http://codereview.chromium.org/576001/diff/3071/387
File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):

http://codereview.chromium.org/576001/diff/3071/387#newcode868
third_party/tcmalloc/chromium/src/tcmalloc.cc:868: // TODO(willchan):
Investigate whether or not lining this much is harmful to
nit: typo lining --> inlining

http://codereview.chromium.org/576001/diff/3071/404
File third_party/tcmalloc/chromium/src/tests/atomicops_unittest.cc (right):

http://codereview.chromium.org/576001/diff/3071/404#newcode63
third_party/tcmalloc/chromium/src/tests/atomicops_unittest.cc:63: ASSERT_EQ(1,
base::subtle::NoBarrier_AtomicIncrement(&s.count, 1));
I believe another part of this CL created synonyms... so I'm surprised to see
the names explicitly changed as well.

Do you really need both synonyms and this name change?

[Alexander Potapenko, 2010-02-10 16:11:10]

http://codereview.chromium.org/576001/diff/3071/401
File third_party/tcmalloc/chromium/src/page_heap.cc (left):

http://codereview.chromium.org/576001/diff/3071/401#oldcode154
third_party/tcmalloc/chromium/src/page_heap.cc:154: void
PageHeap::CommitSpan(Span* span) {
On 2010/02/09 19:30:55, jar wrote:
> As I mentioned in the header... I'm pretty sure these functions are needed.

Retained CommitSpan and DecommitSpan.

http://codereview.chromium.org/576001/diff/3071/401#oldcode195
third_party/tcmalloc/chromium/src/page_heap.cc:195: CommitSpan(span);
On 2010/02/09 19:30:55, jar wrote:
> This call is probably needed.

I've retained it, skipping the free_pages counter.

http://codereview.chromium.org/576001/diff/3071/401#oldcode197
third_party/tcmalloc/chromium/src/page_heap.cc:197: ASSERT(span->location ==
Span::IN_USE);
On 2010/02/09 19:30:55, jar wrote:
> I think these asserts are valuable, given the extra handling of committed and
> non-committed spans in the code.

Retained.

http://codereview.chromium.org/576001/diff/3071/401#oldcode220
third_party/tcmalloc/chromium/src/page_heap.cc:220: // or after we coalesce
them.  The current code always decomits. This is
On 2010/02/09 13:53:43, antonm1 wrote:
> and why these changes are undone?  did you just copy over?
> 
> Please, note, that we might want to undo them given new release mechanism, but
> I'd like to have it discussed first.

I've retained these.

http://codereview.chromium.org/576001/diff/3071/401#oldcode230
third_party/tcmalloc/chromium/src/page_heap.cc:230: // based on memory usage and
free heap sizes.
On 2010/02/09 19:30:55, jar wrote:
> +1 for antonm's comment above.
> This handling of committed and decommitted memmory, and theses comments, needs
> to be preserved.

Done.

http://codereview.chromium.org/576001/diff/3071/401#oldcode239
third_party/tcmalloc/chromium/src/page_heap.cc:239: if (prev->location ==
Span::ON_RETURNED_FREELIST) {
On 2010/02/09 19:30:55, jar wrote:
> Please retain this code (and line 235)

Done.

http://codereview.chromium.org/576001/diff/3071/401#oldcode280
third_party/tcmalloc/chromium/src/page_heap.cc:280: IncrementalScavenge(n);
Should we call IncrementalScavenge from here? I suppose no, as it was in the
previous versions of tcmalloc and was removed recently.

http://codereview.chromium.org/576001/diff/3071/401#oldcode447
third_party/tcmalloc/chromium/src/page_heap.cc:447: committed_bytes_ += (ask <<
kPageShift);
On 2010/02/09 13:53:43, antonm1 wrote:
> and why this is removed

Fixed. I've moved committed_bytes_ to stats_.

http://codereview.chromium.org/576001/diff/3071/401
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/3071/401#newcode272
third_party/tcmalloc/chromium/src/page_heap.cc:272: const double mult = 1000.0 /
rate;
If I understand you correctly, I should keep the perftools' version of
IncrementalScavenge and tune the tcmalloc_release_rate to a satisfactory value.
I'll do.
On 2010/02/09 19:30:55, jar wrote:
> As we move to a new reclamation approach, be very careful to tune for our
client
> environment.  Unlike servers, which have a fairly uniform task and memory
load,
> browsers have rapidly varying load, and hence need to be more responsive (and
> can't assume a nice smooth asymptote).

http://codereview.chromium.org/576001/diff/3071/392
File third_party/tcmalloc/chromium/src/page_heap.h (left):

http://codereview.chromium.org/576001/diff/3071/392#oldcode74
third_party/tcmalloc/chromium/src/page_heap.h:74: typedef
TCMalloc_PageMap1_LazyCommit<32-kPageShift> Type;
On 2010/02/09 19:30:55, jar wrote:
> This code should not be discarded.  If anything, we need to push it back
> upstream.
> 
> Google3 probably doesn't care much about 32 bit systems, but for us, this is a
> big deal.  The savings in meta-data allocation should be several megs, which
can
> add up when there are multiple processes running on a desktop.

Done. Also updated the TCMalloc_PageMap1_LazyCommit declaration in pagemap.h

http://codereview.chromium.org/576001/diff/3071/392#oldcode231
third_party/tcmalloc/chromium/src/page_heap.h:231: void CommitSpan(Span* span);
On 2010/02/09 19:30:55, jar wrote:
> Windows needs actual commits, while linux seems do do such automatically (on
> demand).  I suspect you need to keep thees calls to commit and decommitSpan
> around... but I need to look at the rest of this CL to be sure.

As we've joined the Windows and Linux versions, thes ifdef should be removed, I
suppose.

http://codereview.chromium.org/576001/diff/3071/387
File third_party/tcmalloc/chromium/src/tcmalloc.cc (left):

http://codereview.chromium.org/576001/diff/3071/387#oldcode428
third_party/tcmalloc/chromium/src/tcmalloc.cc:428: stats.committed_bytes /
static_cast<double>(bytes_in_use));
On 2010/02/09 19:30:55, jar wrote:
> I think all these stats were intentionally added... as the relate to committed
> and decommitted issues.  Please preserve them.

Done.

http://codereview.chromium.org/576001/diff/3071/387#oldcode456
third_party/tcmalloc/chromium/src/tcmalloc.cc:456: "MALLOC: %12" PRIu64 " (%7.1f
MB) Bytes committed\n"
On 2010/02/09 19:30:55, jar wrote:
> Please preserve.

Done.

http://codereview.chromium.org/576001/diff/3071/387#oldcode467
third_party/tcmalloc/chromium/src/tcmalloc.cc:467: stats.committed_bytes,
stats.committed_bytes / MB,
On 2010/02/09 19:30:55, jar wrote:
> This is the data source to preserve.

Done, taken from stats.pageheap.

http://codereview.chromium.org/576001/diff/3071/387
File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):

http://codereview.chromium.org/576001/diff/3071/387#newcode868
third_party/tcmalloc/chromium/src/tcmalloc.cc:868: // TODO(willchan):
Investigate whether or not lining this much is harmful to
On 2010/02/09 19:30:55, jar wrote:
> nit: typo lining --> inlining

Done.

http://codereview.chromium.org/576001/diff/3071/404
File third_party/tcmalloc/chromium/src/tests/atomicops_unittest.cc (right):

http://codereview.chromium.org/576001/diff/3071/404#newcode63
third_party/tcmalloc/chromium/src/tests/atomicops_unittest.cc:63: ASSERT_EQ(1,
base::subtle::NoBarrier_AtomicIncrement(&s.count, 1));
Not sure that I understand you.
The last diff of atomicops_unittest.cc is equal to this one
(http://code.google.com/p/google-perftools/source/diff?spec=svn87&r=79&format=...).
Do we have any local changes to this file?
On 2010/02/09 19:30:55, jar wrote:
> I believe another part of this CL created synonyms... so I'm surprised to see
> the names explicitly changed as well.
> 
> Do you really need both synonyms and this name change?

[Alexander Potapenko, 2010-02-10 16:22:37]

On 2010/02/09 14:37:33, antonm1 wrote:
> 
> For me the most crucial are three benchmarks:
> 
> http://dromaeo.com/?dom
> http://dromaeo.com/?jslib
> and DOM benchmark (please, check it out from
> svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would be
> any troubles with access, I could just share it with you).
> 
> Ideally there should be no regressions.  Total scores are most
> important, but ideally we don't regress on any (most) of the tests.
> 
> Good luck with merge and thanks a lot for doing this.

Anton, do we have any reference points for those benchmarks?

[antonm, 2010-02-10 18:36:58]

On Wed, Feb 10, 2010 at 7:22 PM,  <glider@chromium.org> wrote:
> On 2010/02/09 14:37:33, antonm1 wrote:
>
>> For me the most crucial are three benchmarks:
>
>> http://dromaeo.com/?dom
>> http://dromaeo.com/?jslib
>> and DOM benchmark (please, check it out from
>> svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would be
>> any troubles with access, I could just share it with you).
>
>> Ideally there should be no regressions.  Total scores are most
>> important, but ideally we don't regress on any (most) of the tests.
>
>> Good luck with merge and thanks a lot for doing this.
>
> Anton, do we have any reference points for those benchmarks?

What do you mean by that?

The numbers you'll get are affected by your box, etc.  So the only way
to see the effect is to run benchmarks with and without the change.

yours,
anton.

[jar (doing other things), 2010-02-10 18:53:02]

The following is a good way to get benchmark results with less pain:

a) Make a copy of your CL in a fresh tree (so you don't lose the CL history
you've been accumulating).  You can do this easily by creating a tree,
downloading the patch, and then running "patch -p2 <the_patch_file" while
sitting in the src directory.

b) Wait till late at night, when no one else is hammering the tree (this
avoids conflating your results with other landings, and makes it less likely
than anyone will be pulling a tree at the time).

c) Land your change from that temporary tree, and wait about 60 seconds for
all the builders to kick off.

d) revert your change once all the compile builds have started (the
utilitity "drover" is REALLY good at this!!!).

e) Read all the perf results on all the buildbots.  See how good/bad the
results were.

YMMV.

Antonm: Are the tests you are most concerned with present in the buildbot
series??

Jim

On Wed, Feb 10, 2010 at 10:36 AM, Anton Muhin <antonm@chromium.org> wrote:

> On Wed, Feb 10, 2010 at 7:22 PM,  <glider@chromium.org> wrote:
> > On 2010/02/09 14:37:33, antonm1 wrote:
> >
> >> For me the most crucial are three benchmarks:
> >
> >> http://dromaeo.com/?dom
> >> http://dromaeo.com/?jslib
> >> and DOM benchmark (please, check it out from
> >> svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would be
> >> any troubles with access, I could just share it with you).
> >
> >> Ideally there should be no regressions.  Total scores are most
> >> important, but ideally we don't regress on any (most) of the tests.
> >
> >> Good luck with merge and thanks a lot for doing this.
> >
> > Anton, do we have any reference points for those benchmarks?
>
> What do you mean by that?
>
> The numbers you'll get are affected by your box, etc.  So the only way
> to see the effect is to run benchmarks with and without the change.
>
> yours,
> anton.
>

[antonm, 2010-02-10 18:59:59]

Yes, so it might be indeed more convenient.  The only catch---numbers
are not quite stable, so I personally prefer to do several runs on my
box first, by YMMV.

yours,
anton.

On Wed, Feb 10, 2010 at 9:52 PM, Jim Roskind <jar@chromium.org> wrote:
> The following is a good way to get benchmark results with less pain:
> a) Make a copy of your CL in a fresh tree (so you don't lose the CL history
> you've been accumulating).  You can do this easily by creating a tree,
> downloading the patch, and then running "patch -p2 <the_patch_file" while
> sitting in the src directory.
> b) Wait till late at night, when no one else is hammering the tree (this
> avoids conflating your results with other landings, and makes it less likely
> than anyone will be pulling a tree at the time).
> c) Land your change from that temporary tree, and wait about 60 seconds for
> all the builders to kick off.
> d) revert your change once all the compile builds have started (the
> utilitity "drover" is REALLY good at this!!!).
> e) Read all the perf results on all the buildbots.  See how good/bad the
> results were.
> YMMV.
> Antonm: Are the tests you are most concerned with present in the buildbot
> series??
> Jim
>
> On Wed, Feb 10, 2010 at 10:36 AM, Anton Muhin <antonm@chromium.org> wrote:
>>
>> On Wed, Feb 10, 2010 at 7:22 PM,  <glider@chromium.org> wrote:
>> > On 2010/02/09 14:37:33, antonm1 wrote:
>> >
>> >> For me the most crucial are three benchmarks:
>> >
>> >> http://dromaeo.com/?dom
>> >> http://dromaeo.com/?jslib
>> >> and DOM benchmark (please, check it out from
>> >> svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would be
>> >> any troubles with access, I could just share it with you).
>> >
>> >> Ideally there should be no regressions.  Total scores are most
>> >> important, but ideally we don't regress on any (most) of the tests.
>> >
>> >> Good luck with merge and thanks a lot for doing this.
>> >
>> > Anton, do we have any reference points for those benchmarks?
>>
>> What do you mean by that?
>>
>> The numbers you'll get are affected by your box, etc.  So the only way
>> to see the effect is to run benchmarks with and without the change.
>>
>> yours,
>> anton.
>
>

[Alexander Potapenko, 2010-02-11 16:41:24]

I've committed the patch as r38766 and reverted it as r38772.
According to http://build.chromium.org/buildbot/perf/dashboard/overview.html
there was no performance regression.

Jim, should I change anything the tcmalloc_release_rate flag value assuming we
move to the new strategy? Do the perf tests model the memory load good enough?

On 2010/02/10 18:59:59, antonm wrote:
> Yes, so it might be indeed more convenient.  The only catch---numbers
> are not quite stable, so I personally prefer to do several runs on my
> box first, by YMMV.
> 
> yours,
> anton.
> 
> On Wed, Feb 10, 2010 at 9:52 PM, Jim Roskind <mailto:jar@chromium.org> wrote:
> > The following is a good way to get benchmark results with less pain:
> > a) Make a copy of your CL in a fresh tree (so you don't lose the CL history
> > you've been accumulating). &nbsp;You can do this&nbsp;easily&nbsp;by
creating a tree,
> > downloading the patch, and then running "patch -p2 <the_patch_file" while
> > sitting in the src directory.
> > b) Wait till late at night, when no one else is hammering the tree (this
> > avoids conflating your results with other landings, and makes it less likely
> > than anyone will be pulling a tree at the time).
> > c) Land your change from that temporary tree, and wait about 60 seconds for
> > all the builders to kick off.
> > d) revert your change once all the compile builds have started (the
> > utilitity "drover" is REALLY good at this!!!).
> > e) Read all the perf results on all the buildbots. &nbsp;See how good/bad
the
> > results were.
> > YMMV.
> > Antonm: Are the tests you are most concerned with present in the buildbot
> > series??
> > Jim
> >
> > On Wed, Feb 10, 2010 at 10:36 AM, Anton Muhin <mailto:antonm@chromium.org>
wrote:
> >>
> >> On Wed, Feb 10, 2010 at 7:22 PM, mailto:%C2%A0%3Cglider@chromium.org>
wrote:
> >> > On 2010/02/09 14:37:33, antonm1 wrote:
> >> >
> >> >> For me the most crucial are three benchmarks:
> >> >
> >> >> http://dromaeo.com/?dom
> >> >> http://dromaeo.com/?jslib
> >> >> and DOM benchmark (please, check it out from
> >> >> svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would be
> >> >> any troubles with access, I could just share it with you).
> >> >
> >> >> Ideally there should be no regressions. &nbsp;Total scores are most
> >> >> important, but ideally we don't regress on any (most) of the tests.
> >> >
> >> >> Good luck with merge and thanks a lot for doing this.
> >> >
> >> > Anton, do we have any reference points for those benchmarks?
> >>
> >> What do you mean by that?
> >>
> >> The numbers you'll get are affected by your box, etc. &nbsp;So the only way
> >> to see the effect is to run benchmarks with and without the change.
> >>
> >> yours,
> >> anton.
> >
> >
>

[antonm, 2010-02-11 16:45:55]

The cycle might have been too short.  For example
http://build.chromium.org/buildbot/perf/xp-release-dual-core/dromaeo/report.h...
collected only three samples which look something very much like -3%
performance lost.

yours,
anton.

On Thu, Feb 11, 2010 at 7:41 PM,  <glider@chromium.org> wrote:
> I've committed the patch as r38766 and reverted it as r38772.
> According to http://build.chromium.org/buildbot/perf/dashboard/overview.html
> there was no performance regression.
>
> Jim, should I change anything the tcmalloc_release_rate flag value assuming
> we
> move to the new strategy? Do the perf tests model the memory load good
> enough?
>
> On 2010/02/10 18:59:59, antonm wrote:
>>
>> Yes, so it might be indeed more convenient.  The only catch---numbers
>> are not quite stable, so I personally prefer to do several runs on my
>> box first, by YMMV.
>
>> yours,
>> anton.
>
>> On Wed, Feb 10, 2010 at 9:52 PM, Jim Roskind <mailto:jar@chromium.org>
>> wrote:
>> > The following is a good way to get benchmark results with less pain:
>> > a) Make a copy of your CL in a fresh tree (so you don't lose the CL
>> > history
>> > you've been accumulating). &nbsp;You can do this&nbsp;easily&nbsp;by
>
> creating a tree,
>>
>> > downloading the patch, and then running "patch -p2 <the_patch_file"
>> > while
>> > sitting in the src directory.
>> > b) Wait till late at night, when no one else is hammering the tree (this
>> > avoids conflating your results with other landings, and makes it less
>> > likely
>> > than anyone will be pulling a tree at the time).
>> > c) Land your change from that temporary tree, and wait about 60 seconds
>> > for
>> > all the builders to kick off.
>> > d) revert your change once all the compile builds have started (the
>> > utilitity "drover" is REALLY good at this!!!).
>> > e) Read all the perf results on all the buildbots. &nbsp;See how
>> > good/bad
>
> the
>>
>> > results were.
>> > YMMV.
>> > Antonm: Are the tests you are most concerned with present in the
>> > buildbot
>> > series??
>> > Jim
>> >
>> > On Wed, Feb 10, 2010 at 10:36 AM, Anton Muhin
>> > <mailto:antonm@chromium.org>
>
> wrote:
>>
>> >>
>> >> On Wed, Feb 10, 2010 at 7:22 PM, mailto:%C2%A0%3Cglider@chromium.org>
>
> wrote:
>>
>> >> > On 2010/02/09 14:37:33, antonm1 wrote:
>> >> >
>> >> >> For me the most crucial are three benchmarks:
>> >> >
>> >> >> http://dromaeo.com/?dom
>> >> >> http://dromaeo.com/?jslib
>> >> >> and DOM benchmark (please, check it out from
>> >> >> svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would
>> >> >> be
>> >> >> any troubles with access, I could just share it with you).
>> >> >
>> >> >> Ideally there should be no regressions. &nbsp;Total scores are most
>> >> >> important, but ideally we don't regress on any (most) of the tests.
>> >> >
>> >> >> Good luck with merge and thanks a lot for doing this.
>> >> >
>> >> > Anton, do we have any reference points for those benchmarks?
>> >>
>> >> What do you mean by that?
>> >>
>> >> The numbers you'll get are affected by your box, etc. &nbsp;So the only
>> >> way
>> >> to see the effect is to run benchmarks with and without the change.
>> >>
>> >> yours,
>> >> anton.
>> >
>> >
>
>
>
>
> http://codereview.chromium.org/576001
>

[jar (doing other things), 2010-02-11 19:05:19]

+to: mbelshe

IMO, the perf bots are not proof-positive that all is well, but are at least
excellent warning signs that we've really messed up (if they go south).

In the page-cycle-moz, XP-dual-core, I think it looks like a 40ms regression
during your checkin period.  Do you have an understanding of what that was
about? It would be nice to see a clean in/out with no changes over that
period.  As I mentioned, it is optimal to land separately from other folks
(you got bundled in).  I think your landing was clear... but separate data
is always best (you don't want a perf improvement from one landing to hide
regression from another).

Did you also carefully look at all the memory perf results? (I actually
did... but when I looked at them I saw a GIANT regression from a different
landing... which made me wonder if you had checked those graphs out as
well).

You are dead on about the fact that I don't believe our tests really "test
under load," and we certainly don't "test for an extended period of time."

mbelshe: Do you have suggestions for things we might want to try to help
validate things are cool?  I recall efforts to clean up release of memory...
but I'm having a hard time remembering the specific exercise test cases.


Thanks,

Jim


On Thu, Feb 11, 2010 at 8:41 AM, <glider@chromium.org> wrote:

> I've committed the patch as r38766 and reverted it as r38772.
> According to
> http://build.chromium.org/buildbot/perf/dashboard/overview.html
> there was no performance regression.
>
> Jim, should I change anything the tcmalloc_release_rate flag value assuming
> we
> move to the new strategy? Do the perf tests model the memory load good
> enough?
>
>
> On 2010/02/10 18:59:59, antonm wrote:
>
>> Yes, so it might be indeed more convenient.  The only catch---numbers
>> are not quite stable, so I personally prefer to do several runs on my
>> box first, by YMMV.
>>
>
>  yours,
>> anton.
>>
>
>  On Wed, Feb 10, 2010 at 9:52 PM, Jim Roskind <mailto:jar@chromium.org>
>> wrote:
>> > The following is a good way to get benchmark results with less pain:
>> > a) Make a copy of your CL in a fresh tree (so you don't lose the CL
>> history
>> > you've been accumulating). &nbsp;You can do this&nbsp;easily&nbsp;by
>>
> creating a tree,
>
>> > downloading the patch, and then running "patch -p2 <the_patch_file"
>> while
>> > sitting in the src directory.
>> > b) Wait till late at night, when no one else is hammering the tree (this
>> > avoids conflating your results with other landings, and makes it less
>> likely
>> > than anyone will be pulling a tree at the time).
>> > c) Land your change from that temporary tree, and wait about 60 seconds
>> for
>> > all the builders to kick off.
>> > d) revert your change once all the compile builds have started (the
>> > utilitity "drover" is REALLY good at this!!!).
>> > e) Read all the perf results on all the buildbots. &nbsp;See how
>> good/bad
>>
> the
>
>> > results were.
>> > YMMV.
>> > Antonm: Are the tests you are most concerned with present in the
>> buildbot
>> > series??
>> > Jim
>> >
>> > On Wed, Feb 10, 2010 at 10:36 AM, Anton Muhin <mailto:
>> antonm@chromium.org>
>>
> wrote:
>
>> >>
>> >> On Wed, Feb 10, 2010 at 7:22 PM,
mailto:%C2%A0%3Cglider@chromium.org<C2%25A0%253Cglider@chromium.org>
>> >
>>
> wrote:
>
>> >> > On 2010/02/09 14:37:33, antonm1 wrote:
>> >> >
>> >> >> For me the most crucial are three benchmarks:
>> >> >
>> >> >> http://dromaeo.com/?dom
>> >> >> http://dromaeo.com/?jslib
>> >> >> and DOM benchmark (please, check it out from
>> >> >> svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would
>> be
>> >> >> any troubles with access, I could just share it with you).
>> >> >
>> >> >> Ideally there should be no regressions. &nbsp;Total scores are most
>>
>> >> >> important, but ideally we don't regress on any (most) of the tests.
>> >> >
>> >> >> Good luck with merge and thanks a lot for doing this.
>> >> >
>> >> > Anton, do we have any reference points for those benchmarks?
>> >>
>> >> What do you mean by that?
>> >>
>> >> The numbers you'll get are affected by your box, etc. &nbsp;So the only
>> way
>>
>> >> to see the effect is to run benchmarks with and without the change.
>> >>
>> >> yours,
>> >> anton.
>> >
>> >
>>
>
>
>
>
> http://codereview.chromium.org/576001
>

[_com_google_glider, 2010-02-11 20:27:54]

Thanks, I'll take an extended look tomorrow morning.
On 2010/02/11 16:45:55, antonm wrote:
> The cycle might have been too short.  For example
>
http://build.chromium.org/buildbot/perf/xp-release-dual-core/dromaeo/report.h...
> collected only three samples which look something very much like -3%
> performance lost.
> 
> yours,
> anton.
> 
> On Thu, Feb 11, 2010 at 7:41 PM,  <mailto:glider@chromium.org> wrote:
> > I've committed the patch as r38766 and reverted it as r38772.
> > According to http://build.chromium.org/buildbot/perf/dashboard/overview.html
> > there was no performance regression.
> >
> > Jim, should I change anything the tcmalloc_release_rate flag value assuming
> > we
> > move to the new strategy? Do the perf tests model the memory load good
> > enough?
> >
> > On 2010/02/10 18:59:59, antonm wrote:
> >>
> >> Yes, so it might be indeed more convenient. &nbsp;The only catch---numbers
> >> are not quite stable, so I personally prefer to do several runs on my
> >> box first, by YMMV.
> >
> >> yours,
> >> anton.
> >
> >> On Wed, Feb 10, 2010 at 9:52 PM, Jim Roskind <mailto:jar@chromium.org>
> >> wrote:
> >> > The following is a good way to get benchmark results with less pain:
> >> > a) Make a copy of your CL in a fresh tree (so you don't lose the CL
> >> > history
> >> > you've been accumulating). &nbsp;You can do this&nbsp;easily&nbsp;by
> >
> > creating a tree,
> >>
> >> > downloading the patch, and then running "patch -p2 <the_patch_file"
> >> > while
> >> > sitting in the src directory.
> >> > b) Wait till late at night, when no one else is hammering the tree (this
> >> > avoids conflating your results with other landings, and makes it less
> >> > likely
> >> > than anyone will be pulling a tree at the time).
> >> > c) Land your change from that temporary tree, and wait about 60 seconds
> >> > for
> >> > all the builders to kick off.
> >> > d) revert your change once all the compile builds have started (the
> >> > utilitity "drover" is REALLY good at this!!!).
> >> > e) Read all the perf results on all the buildbots. &nbsp;See how
> >> > good/bad
> >
> > the
> >>
> >> > results were.
> >> > YMMV.
> >> > Antonm: Are the tests you are most concerned with present in the
> >> > buildbot
> >> > series??
> >> > Jim
> >> >
> >> > On Wed, Feb 10, 2010 at 10:36 AM, Anton Muhin
> >> > <mailto:antonm@chromium.org>
> >
> > wrote:
> >>
> >> >>
> >> >> On Wed, Feb 10, 2010 at 7:22 PM, mailto:%C2%A0%3Cglider@chromium.org>
> >
> > wrote:
> >>
> >> >> > On 2010/02/09 14:37:33, antonm1 wrote:
> >> >> >
> >> >> >> For me the most crucial are three benchmarks:
> >> >> >
> >> >> >> http://dromaeo.com/?dom
> >> >> >> http://dromaeo.com/?jslib
> >> >> >> and DOM benchmark (please, check it out from
> >> >> >> svn://chrome-svn/chrome-internal/trunk/data/dom_perf if there would
> >> >> >> be
> >> >> >> any troubles with access, I could just share it with you).
> >> >> >
> >> >> >> Ideally there should be no regressions. &nbsp;Total scores are most
> >> >> >> important, but ideally we don't regress on any (most) of the tests.
> >> >> >
> >> >> >> Good luck with merge and thanks a lot for doing this.
> >> >> >
> >> >> > Anton, do we have any reference points for those benchmarks?
> >> >>
> >> >> What do you mean by that?
> >> >>
> >> >> The numbers you'll get are affected by your box, etc. &nbsp;So the only
> >> >> way
> >> >> to see the effect is to run benchmarks with and without the change.
> >> >>
> >> >> yours,
> >> >> anton.
> >> >
> >> >
> >
> >
> >
> >
> > http://codereview.chromium.org/576001
> >
>

[_com_google_glider, 2010-02-11 20:31:50]

I actually think that it's better to keep the patch landed for several revisions
to get better statistics. Unfortunately it's really easy to mess up with someone
else's CL because the team is too distributed :)

I'll try to land the patch again tomorrow morning (Moscow) and look at the stats
again. I am also going to run the benchmarks locally.

Alex

[Alexander Potapenko, 2010-02-12 11:51:21]

I've ran the benchmarks from http://dromaeo.com/?dom with and without the
tcmalloc patch on my Linux box (Mozilla/5.0 (X11; U; Linux x86_64; en-US)
AppleWebKit/533.1 (KHTML, like Gecko) Chrome/5.0.325.0 Safari/533.1), the links
to the results are below (sorted by the Total value).

In general, the performance of the patched version is as good as of the
unpatched one. Also, for each benchmark result ran on the unpatched Chromium I
could find a better result on the patched Chromium.

Other test results will follow.

Benchmark results for the unpatched version (old tcmalloc):
http://dromaeo.com/?id=91831
http://dromaeo.com/?id=91833
http://dromaeo.com/?id=91828
http://dromaeo.com/?id=91830
http://dromaeo.com/?id=91836
http://dromaeo.com/?id=91823
http://dromaeo.com/?id=91832
http://dromaeo.com/?id=91822
http://dromaeo.com/?id=91818
http://dromaeo.com/?id=91834

Benchmark for the patched version (new tcmalloc):
http://dromaeo.com/?id=91825
http://dromaeo.com/?id=91819
http://dromaeo.com/?id=91843
http://dromaeo.com/?id=91824
http://dromaeo.com/?id=91826
http://dromaeo.com/?id=91845
http://dromaeo.com/?id=91848
http://dromaeo.com/?id=91837
http://dromaeo.com/?id=91846
http://dromaeo.com/?id=91840
http://dromaeo.com/?id=91817

[Alexander Potapenko, 2010-02-12 11:55:45]

> Also, for each benchmark result ran on the unpatched Chromium I
> could find a better result on the patched Chromium.

Well, actually for some of them that's true only considering that benchmark's
deviation.

[Anton Muhin, 2010-02-12 13:10:47]

Sorry, but I am under impression that tcmalloc is not enabled on Linux.

Plus committing/decommitting layer in Win port is quite different from
Linux, so I think it says us nothing about perf on Win.

yours,
anton.

On Fri, Feb 12, 2010 at 2:55 PM,  <glider@chromium.org> wrote:
>
>> Also, for each benchmark result ran on the unpatched Chromium I
>> could find a better result on the patched Chromium.
>
> Well, actually for some of them that's true only considering that
> benchmark's
> deviation.
>
> http://codereview.chromium.org/576001
>

[Alexander Potapenko, 2010-02-12 13:27:50]

On 2010/02/12 13:10:47, antonm1 wrote:
> Sorry, but I am under impression that tcmalloc is not enabled on Linux.
> 
Both of the binaries report the presence of the heap leak checker (and thus
tcmalloc) when I run:
$ HEAPCHECK=strict sconsbuild/Release/chrome
, so I believe it's enabled.

> Plus committing/decommitting layer in Win port is quite different from
> Linux, so I think it says us nothing about perf on Win.
> 

That's true. I've committed the patch again as r38882 and am waiting for a
couple of test runs to pass.

[willchan no longer on Chromium, 2010-02-12 17:45:59]

On Fri, Feb 12, 2010 at 5:27 AM,  <glider@chromium.org> wrote:
> On 2010/02/12 13:10:47, antonm1 wrote:
>>
>> Sorry, but I am under impression that tcmalloc is not enabled on Linux.
>
> Both of the binaries report the presence of the heap leak checker (and thus
> tcmalloc) when I run:
> $ HEAPCHECK=strict sconsbuild/Release/chrome
> , so I believe it's enabled.

The linux_use_tcmalloc gyp variable is set to 0 by default in
common.gypi.  Are you overriding that?  I forget the settings wrt
heapchecker, but I suspect you might have enabled tcmalloc by enabling
heapchecker in your local build.

>
>> Plus committing/decommitting layer in Win port is quite different from
>> Linux, so I think it says us nothing about perf on Win.
>
>
> That's true. I've committed the patch again as r38882 and am waiting for a
> couple of test runs to pass.
>
> http://codereview.chromium.org/576001
>

[_com_google_glider, 2010-02-12 19:03:05]

> The linux_use_tcmalloc gyp variable is set to 0 by default in
> common.gypi.  Are you overriding that?  I forget the settings wrt
> heapchecker, but I suspect you might have enabled tcmalloc by enabling
> heapchecker in your local build.

Thanks for reminding that. I've really been building all my binaries with
tcmalloc enabled for a long time. Anyway, if we're going to commit this CL
someday, my local tests can help to make sure we're not breaking the Linux
support. It would be better to check that on the bots, however.

[Alexander Potapenko, 2010-02-16 09:41:29]

I've reverted the CL again (r39083). Will check the performance results now.

[Alexander Potapenko, 2010-02-16 11:42:56]

> In the page-cycle-moz, XP-dual-core, I think it looks like a 40ms regression
> during your checkin period.  Do you have an understanding of what that was
> about? It would be nice to see a clean in/out with no changes over that
> period.  As I mentioned, it is optimal to land separately from other folks
> (you got bundled in).  I think your landing was clear... but separate data
> is always best (you don't want a perf improvement from one landing to hide
> regression from another).
> 
> Did you also carefully look at all the memory perf results? (I actually
> did... but when I looked at them I saw a GIANT regression from a different
> landing... which made me wonder if you had checked those graphs out as
> well).
Looking at the newer landing results (r38882 to r39083) I suppose that the
results during the extended period of using my patch are very much alike to the
results collected without this patch. There still exist some outstanding values,
but the mean's behavior for the memory tests remains unaffected by my commits.

[Alexander Potapenko, 2010-02-18 10:47:00]

Should I proceed with committing this CL or we should test anything else before?

[antonm, 2010-02-18 11:08:15]

I checked our bots graphs for benchmarks and everything looks fine---thanks
a lot for checking it, Alexander.

Let me make another pass over CL.

yours,
anton.

On Thu, Feb 18, 2010 at 1:47 PM, <glider@chromium.org> wrote:

> Should I proceed with committing this CL or we should test anything else
> before?
>
>
> http://codereview.chromium.org/576001
>

[antonm, 2010-02-18 11:30:07]

Mostly lgtm.  But please wait for at least for Jim's (jar@) blessing if nobody
else would like to speak up.

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/14002/13075#newcode178
third_party/tcmalloc/chromium/src/page_heap.cc:178: PrependToFreeList(leftover);
 // Skip coalescing - no candidates possible
should ASSERT be added to prove it?

http://codereview.chromium.org/576001/diff/14002/13075#newcode204
third_party/tcmalloc/chromium/src/page_heap.cc:204: IncrementalScavenge(n);
have you added IncrementalScavenge (I used to think we disabled that)?

If yes, it might be not the best idea as all the pages are decommitted in free
list and I am not sure we need scavenging at all.

http://codereview.chromium.org/576001/diff/14002/13075#newcode283
third_party/tcmalloc/chromium/src/page_heap.cc:283: stats_.unmapped_bytes +=
(span->length << kPageShift);
is it true that stats_.unmapped_bytes + stats_.committed_bytes ==
stats_.system_bytes?  If yes, could we either ASSERT that, or calculate one from
another.

Plus a mix of windows language (committed) and *nix (unmapped) looks somewhat
weird to me.

http://codereview.chromium.org/576001/diff/14002/13097
File third_party/tcmalloc/chromium/src/thread_cache.cc (left):

http://codereview.chromium.org/576001/diff/14002/13097#oldcode302
third_party/tcmalloc/chromium/src/thread_cache.cc:302: // There is a slight
potential race here because of double-checked
just curious, why it removed?

[Alexander Potapenko, 2010-02-18 18:15:42]

(Anton, I will address the rest of your comments a bit later, sorry)

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/14002/13075#newcode204
third_party/tcmalloc/chromium/src/page_heap.cc:204: IncrementalScavenge(n);
On 2010/02/18 11:30:07, antonm wrote:
> have you added IncrementalScavenge (I used to think we disabled that)?
> 
> If yes, it might be not the best idea as all the pages are decommitted in free
> list and I am not sure we need scavenging at all.

Please take a look at the current version of the code.
PageHeap::Delete() calls IncrementalScavenge() at the very end.

http://codereview.chromium.org/576001/diff/14002/13097
File third_party/tcmalloc/chromium/src/thread_cache.cc (left):

http://codereview.chromium.org/576001/diff/14002/13097#oldcode302
third_party/tcmalloc/chromium/src/thread_cache.cc:302: // There is a slight
potential race here because of double-checked
On 2010/02/18 11:30:07, antonm wrote:
> just curious, why it removed?

It's not accurate anymore and was removed from perftools as well. I suppose
kash@ has fixed that.

[jar (doing other things), 2010-02-18 19:51:56]

I see a lot of this merge relates to patching code.  I think mbelshe worked
pretty hard to patch in a different way on Windows, and avoid the default
patching code (which proved problematic).  Are we continuing to use the  mbelshe
method?

I also have a few focused questions below.

Thanks,

Jim

http://codereview.chromium.org/576001/diff/14002/13101
File third_party/tcmalloc/chromium/src/base/low_level_alloc.cc (right):

http://codereview.chromium.org/576001/diff/14002/13101#newcode219
third_party/tcmalloc/chromium/src/base/low_level_alloc.cc:219: #if 0
nit: The three #if 0 lines in this section are really coordinated (they should
all be turned on or off together).  If this is a merge from uber code...  I
guess it is fine.  IF not, then how about defining (or undef'ng) a common macro,
and having all of these blocks depend on that common macro?

For example:

#undef ASYNC_SIGNAL_SAFE_SUPPORT
...
#if defined ASYNC_SIGNAL_SAFE_SUPPORT
...

I also wonder about all the other supporting code here (that is really not
effectively enabled), but perchance the minimal perf and bloat impact of that
useless code is not worth the #if protection.

http://codereview.chromium.org/576001/diff/14002/13060
File third_party/tcmalloc/chromium/src/debugallocation.cc (right):

http://codereview.chromium.org/576001/diff/14002/13060#newcode671
third_party/tcmalloc/chromium/src/debugallocation.cc:671: char* pc =
nit: Line wraps not needed (but if this just matches up-stream merge... then OK.

http://codereview.chromium.org/576001/diff/14002/13060#newcode678
third_party/tcmalloc/chromium/src/debugallocation.cc:678: char *pc =
nit: Line wrap not needed.

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (left):

http://codereview.chromium.org/576001/diff/14002/13075#oldcode192
third_party/tcmalloc/chromium/src/page_heap.cc:192: free_pages_ -= n;
I would have expected a corresponding stats.free_pages -= n

http://codereview.chromium.org/576001/diff/14002/13075#oldcode293
third_party/tcmalloc/chromium/src/page_heap.cc:293: static const int
kMaxReleaseDelay = 1 << 14;
Here the math makes sense again.... 
Did you inadvertently change the delay to 64K?

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/14002/13075#newcode334
third_party/tcmalloc/chromium/src/page_heap.cc:334:
TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
I think spans get coalesced... but sometimes, when we try to release them, we
can only release a portion, as the postfix or prefix overlaps (on Windows) with
a partial block of a virtual alloc.  I think we have to be especially careful to
round up/down to boundaries much larger than page boundaries (kPageShift == 4K),
and I think we need to worry about 64K boundaries.

Does this code assume that the entire span gets released? Does this lead to
fragmentation? Am I confused?

http://codereview.chromium.org/576001/diff/14002/13066
File third_party/tcmalloc/chromium/src/page_heap.h (right):

http://codereview.chromium.org/576001/diff/14002/13066#newcode199
third_party/tcmalloc/chromium/src/page_heap.h:199: static const int
kMaxReleaseDelay = 1 << 16;
I'm confused by the math here.
Delay is 64K
Page size is 4KB
Product is then 256MB (which I assume is the change from 4GB).

Why does the comment say changed to 64MB?  What was the (old?) chrome intent?

http://codereview.chromium.org/576001/diff/14002/13066#newcode204
third_party/tcmalloc/chromium/src/page_heap.h:204: static const int
kDefaultReleaseDelay = 1 << 12;
Here the math does make sense.
Delay is 4K
Pages are 4KB
Product is 16MB (changed for from from 1GB)

http://codereview.chromium.org/576001/diff/14002/13066#newcode272
third_party/tcmalloc/chromium/src/page_heap.h:272: Length
ReleaseLastNormalSpan(SpanList* slist);
I have to see how this is called etc.... but the comment suggests the semantics
are very different from the old code, which released a full free list
(presumably of a given size), and here it sounds like we just release one entry
(end of list).

[Alexander Potapenko, 2010-02-19 11:06:51]

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/14002/13075#newcode178
third_party/tcmalloc/chromium/src/page_heap.cc:178: PrependToFreeList(leftover);
 // Skip coalescing - no candidates possible
On 2010/02/18 11:30:07, antonm wrote:
> should ASSERT be added to prove it?

I've tried replacing PrependToFreeList() with MergeIntoFreeList(), but it
crashed on the following assertion:
ASSERT(prev->start + prev->length == p);

That means that the prerequisites for coalescing the span are met, but it can't
be done properly. Quite surprising.

http://codereview.chromium.org/576001/diff/14002/13075#newcode283
third_party/tcmalloc/chromium/src/page_heap.cc:283: stats_.unmapped_bytes +=
(span->length << kPageShift);
On 2010/02/18 11:30:07, antonm wrote:
> is it true that stats_.unmapped_bytes + stats_.committed_bytes ==
> stats_.system_bytes?  If yes, could we either ASSERT that, or calculate one
from
> another.
I haven't managed to prove this. Moreover, I've tried to add such an assertion
to Check() and the tests crashed.
> 
> Plus a mix of windows language (committed) and *nix (unmapped) looks somewhat
> weird to me.
This is because this code was forked and existed only in the Windows version. I
can change that if you've got a better idea.

[antonm, 2010-02-19 11:33:29]

Alexander,

sorry it turns to be a hard review.  But the reason is not our nastiness but
rather the crucial role tcmalloc plays.

And thanks a lot for all the hard work you've already done.

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/14002/13075#newcode178
third_party/tcmalloc/chromium/src/page_heap.cc:178: PrependToFreeList(leftover);
 // Skip coalescing - no candidates possible
On 2010/02/19 11:06:51, Alexander Potapenko wrote:
> On 2010/02/18 11:30:07, antonm wrote:
> > should ASSERT be added to prove it?
> 
> I've tried replacing PrependToFreeList() with MergeIntoFreeList(), but it
> crashed on the following assertion:
> ASSERT(prev->start + prev->length == p);
> 
> That means that the prerequisites for coalescing the span are met, but it
can't
> be done properly. Quite surprising.

I wonder if that means something sketchy goes on.  May I ask you to find that
out?  I'd rather not risk tcmalloc stability---those bugs might be very hard to
debug from minidumps we have.

http://codereview.chromium.org/576001/diff/14002/13075#newcode283
third_party/tcmalloc/chromium/src/page_heap.cc:283: stats_.unmapped_bytes +=
(span->length << kPageShift);
On 2010/02/19 11:06:51, Alexander Potapenko wrote:
> On 2010/02/18 11:30:07, antonm wrote:
> > is it true that stats_.unmapped_bytes + stats_.committed_bytes ==
> > stats_.system_bytes?  If yes, could we either ASSERT that, or calculate one
> from
> > another.
> I haven't managed to prove this. Moreover, I've tried to add such an assertion
> to Check() and the tests crashed.
> > 
> > Plus a mix of windows language (committed) and *nix (unmapped) looks
somewhat
> > weird to me.
> This is because this code was forked and existed only in the Windows version.
I
> can change that if you've got a better idea.
> 

Ditto.

[Alexander Potapenko, 2010-02-19 17:36:27]

http://codereview.chromium.org/576001/diff/14002/13101
File third_party/tcmalloc/chromium/src/base/low_level_alloc.cc (right):

http://codereview.chromium.org/576001/diff/14002/13101#newcode219
third_party/tcmalloc/chromium/src/base/low_level_alloc.cc:219: #if 0
On 2010/02/18 19:51:56, jar wrote:
> nit: The three #if 0 lines in this section are really coordinated (they should
> all be turned on or off together).  If this is a merge from uber code...  I
> guess it is fine.  IF not, then how about defining (or undef'ng) a common
macro,
> and having all of these blocks depend on that common macro?
> 
> For example:
> 
> #undef ASYNC_SIGNAL_SAFE_SUPPORT
> ...
> #if defined ASYNC_SIGNAL_SAFE_SUPPORT
> ...
> 
> I also wonder about all the other supporting code here (that is really not
> effectively enabled), but perchance the minimal perf and bloat impact of that
> useless code is not worth the #if protection.
> 

This is a merge from uber code, so I doubt we have to bring more entropy we
need. Let us keep the diff from trunk small.

http://codereview.chromium.org/576001/diff/14002/13060
File third_party/tcmalloc/chromium/src/debugallocation.cc (right):

http://codereview.chromium.org/576001/diff/14002/13060#newcode671
third_party/tcmalloc/chromium/src/debugallocation.cc:671: char* pc =
On 2010/02/18 19:51:56, jar wrote:
> nit: Line wraps not needed (but if this just matches up-stream merge... then
OK.

It does. I think we'd better keep the upstream code structure to prevent future
merge conflicts.

http://codereview.chromium.org/576001/diff/14002/13060#newcode678
third_party/tcmalloc/chromium/src/debugallocation.cc:678: char *pc =
On 2010/02/18 19:51:56, jar wrote:
> nit: Line wrap not needed.
> 

Ditto

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (left):

http://codereview.chromium.org/576001/diff/14002/13075#oldcode192
third_party/tcmalloc/chromium/src/page_heap.cc:192: free_pages_ -= n;
On 2010/02/18 19:51:56, jar wrote:
> I would have expected a corresponding stats.free_pages -= n

|free_pages| counter was removed from PageHeap::Stats and replaced with
|free_bytes| and |unmapped_bytes|.

http://codereview.chromium.org/576001/diff/14002/13075#oldcode293
third_party/tcmalloc/chromium/src/page_heap.cc:293: static const int
kMaxReleaseDelay = 1 << 14;
On 2010/02/18 19:51:56, jar wrote:
> Here the math makes sense again.... 
> Did you inadvertently change the delay to 64K?

Looks like that. Fixed.

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/14002/13075#newcode334
third_party/tcmalloc/chromium/src/page_heap.cc:334:
TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
On 2010/02/18 19:51:56, jar wrote:
> I think spans get coalesced... but sometimes, when we try to release them, we
> can only release a portion, as the postfix or prefix overlaps (on Windows)
with
> a partial block of a virtual alloc.  I think we have to be especially careful
to
> round up/down to boundaries much larger than page boundaries (kPageShift ==
4K),
> and I think we need to worry about 64K boundaries.
> 
> Does this code assume that the entire span gets released? Does this lead to
> fragmentation? Am I confused?

As I understand (see
http://google-perftools.googlecode.com/svn/trunk/doc/tcmalloc.html), tcmalloc
always operates at page grain. When a span is about to be deallocated, it may be
coalesced to the neighbour spans (the previous one and/or the next one) iff they
are unused already. But those spans are page-aligned as well, so it's not a
problem.
I'm not very familliar with the means of memory allocation on Windows, but why
should an object cross a page boundary at all?

http://codereview.chromium.org/576001/diff/14002/13066
File third_party/tcmalloc/chromium/src/page_heap.h (right):

http://codereview.chromium.org/576001/diff/14002/13066#newcode199
third_party/tcmalloc/chromium/src/page_heap.h:199: static const int
kMaxReleaseDelay = 1 << 16;
On 2010/02/18 19:51:56, jar wrote:
> I'm confused by the math here.
> Delay is 64K
> Page size is 4KB
> Product is then 256MB (which I assume is the change from 4GB).
> 
> Why does the comment say changed to 64MB?  What was the (old?) chrome intent?

I guess I've occasionally replaced 1 << 14 with 1 << 16. Fixed.

http://codereview.chromium.org/576001/diff/14002/13066#newcode204
third_party/tcmalloc/chromium/src/page_heap.h:204: static const int
kDefaultReleaseDelay = 1 << 12;
On 2010/02/18 19:51:56, jar wrote:
> Here the math does make sense.
> Delay is 4K
> Pages are 4KB
> Product is 16MB (changed for from from 1GB)

Done.

http://codereview.chromium.org/576001/diff/14002/13066#newcode272
third_party/tcmalloc/chromium/src/page_heap.h:272: Length
ReleaseLastNormalSpan(SpanList* slist);
On 2010/02/18 19:51:56, jar wrote:
> I have to see how this is called etc.... but the comment suggests the
semantics
> are very different from the old code, which released a full free list
> (presumably of a given size), and here it sounds like we just release one
entry
> (end of list).

Isn't it replaced by PageHeap::ReleaseAtLeastNPages()? It calls
ReleaseLastNormalSpan in a loop, adopting the logic removed from
IncrementalScavenge().

[Alexander Potapenko, 2010-02-19 17:38:29]

On 2010/02/18 19:51:56, jar wrote:
> I see a lot of this merge relates to patching code.  I think mbelshe worked
> pretty hard to patch in a different way on Windows, and avoid the default
> patching code (which proved problematic).  Are we continuing to use the 
mbelshe
> method?
I'll check that code again soon but I suppose it shouldn't have been disabled.

[Alexander Potapenko, 2010-02-19 17:41:00]

On 2010/02/19 11:33:29, antonm wrote:
> Alexander,
> 
> sorry it turns to be a hard review.  But the reason is not our nastiness but
> rather the crucial role tcmalloc plays.
>
That's ok.

I'll go on with debugging both cases you've mentioned next week.

[jar (doing other things), 2010-02-19 18:47:58]

We probably need a unit test written, per suggestion below.  It *might* be that
the problem manifested itself with 3 blocks (not just two as in the example),
but I think 2 will suffice.

I'm sorry that we don't already have such a test... and I'm a little wary that
we may have a shot at regressing on this fragmentation front.

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/14002/13075#newcode334
third_party/tcmalloc/chromium/src/page_heap.cc:334:
TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
The problem is that on windows, allocations from the OS (and releases to the OS)
are only done on 64K boundaries.  As a result (for example), if you ask the OS
for 4K, it will reserve 4K, but give you a 64K aligned pointer (effectively
fragmenting VM space, making it impossible for you to coalesce this allocated
region/span with a neighbor).

This class of problems proved to a a BIG issue with the windows port.  We are
very careful to round-up any requests for memory from the OS, to assure that we
get full 64K blocks, and don't skip anything.

I think there is a similar problem with releasing spans (but my mind is a bit
foggy on this).  Note that IF you tried to release a span that wasn't fully 64K
aligned, then the release would probably have to effectively align both ends
(and not release as much as you told it to).  If it did really release all 4K
pages, then it would be nearly impossible for the OS to provide them back in a
VirtualAlloc call, as it ONLY gives 64K aligned blocks, and part of that block
was not released back to it :-(.

We probably need a unit test that verifies that fragmentation is not being
caused by this problem.  I believe we used to have a test case (when we
discovered this problem) that involved a series of progressively larger
allocations and frees.  It was actually antonm who spotted the underlying cause.
 The test scenario was something like:

void *old_pointer = NULL;
for (size_t size = 32768, size < 200 * 1024 * 1024; size += 4096) {
  void *new_pointer = malloc(size);
  free(old_pointer);  // Free *after* you malloc new memory.
  old_pointer = new_pointer;
}

IF the system fragments memory, the above will quickly exhaust memory (and
crash).  If all is well, it will have no problem maintaining a pair of 200M
blocks.

On 2010/02/19 17:36:27, Alexander Potapenko wrote:
> On 2010/02/18 19:51:56, jar wrote:
> > I think spans get coalesced... but sometimes, when we try to release them,
we
> > can only release a portion, as the postfix or prefix overlaps (on Windows)
> with
> > a partial block of a virtual alloc.  I think we have to be especially
careful
> to
> > round up/down to boundaries much larger than page boundaries (kPageShift ==
> 4K),
> > and I think we need to worry about 64K boundaries.
> > 
> > Does this code assume that the entire span gets released? Does this lead to
> > fragmentation? Am I confused?
> 
> As I understand (see
> http://google-perftools.googlecode.com/svn/trunk/doc/tcmalloc.html), tcmalloc
> always operates at page grain. When a span is about to be deallocated, it may
be
> coalesced to the neighbour spans (the previous one and/or the next one) iff
they
> are unused already. But those spans are page-aligned as well, so it's not a
> problem.
> I'm not very familliar with the means of memory allocation on Windows, but why
> should an object cross a page boundary at all?

[Alexander Potapenko, 2010-02-27 16:42:41]

> http://codereview.chromium.org/576001/diff/14002/13075#newcode334
> third_party/tcmalloc/chromium/src/page_heap.cc:334:
> TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
> The problem is that on windows, allocations from the OS (and releases to the
OS)
> are only done on 64K boundaries.  As a result (for example), if you ask the OS
> for 4K, it will reserve 4K, but give you a 64K aligned pointer (effectively
> fragmenting VM space, making it impossible for you to coalesce this allocated
> region/span with a neighbor).
> 
> This class of problems proved to a a BIG issue with the windows port.  We are
> very careful to round-up any requests for memory from the OS, to assure that
we
> get full 64K blocks, and don't skip anything.
> 
> I think there is a similar problem with releasing spans (but my mind is a bit
> foggy on this).  Note that IF you tried to release a span that wasn't fully
64K
> aligned, then the release would probably have to effectively align both ends
> (and not release as much as you told it to).  If it did really release all 4K
> pages, then it would be nearly impossible for the OS to provide them back in a
> VirtualAlloc call, as it ONLY gives 64K aligned blocks, and part of that block
> was not released back to it :-(.
> 
> We probably need a unit test that verifies that fragmentation is not being
> caused by this problem.  I believe we used to have a test case (when we
> discovered this problem) that involved a series of progressively larger
> allocations and frees.  It was actually antonm who spotted the underlying
cause.
>  The test scenario was something like:
> 
> void *old_pointer = NULL;
> for (size_t size = 32768, size < 200 * 1024 * 1024; size += 4096) {
>   void *new_pointer = malloc(size);
>   free(old_pointer);  // Free *after* you malloc new memory.
>   old_pointer = new_pointer;
> }
> 
> IF the system fragments memory, the above will quickly exhaust memory (and
> crash).  If all is well, it will have no problem maintaining a pair of 200M
> blocks.
> 

As far as I understood Anton, this functionality is already implemented in
src/windows/port.cc (see http://codereview.chromium.org/235003/show) -- it
wasn't changed, so we've missed that fix. I've tested this patch on the recent
perftools and it turned out to be enough to decrease the memory consumption
drastically.

[Alexander Potapenko, 2010-03-05 12:27:20]

http://codereview.chromium.org/576001/diff/27001/27024
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/27001/27024#newcode178
third_party/tcmalloc/chromium/src/page_heap.cc:178: PrependToFreeList(leftover);
 // Skip coalescing - no candidates possible
Aha, that's it!
At this point the |span| object has inconsistent length, that's why
MergeIntoFreeList fails. But actually |span| is the previous span for
|leftover|, therefore they can't be coalesced (we've just splitted them and set
different locations for them, so no assertion is needed).
The next object for |leftover| is the span that previously was the next one for
|span|. That's why they can't be coalesced either. We can assert for this.

[Alexander Potapenko, 2010-03-05 15:25:01]

http://codereview.chromium.org/576001/diff/14002/13075
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/14002/13075#newcode283
third_party/tcmalloc/chromium/src/page_heap.cc:283: stats_.unmapped_bytes +=
(span->length << kPageShift);
On 2010/02/19 11:33:29, antonm wrote:
> On 2010/02/19 11:06:51, Alexander Potapenko wrote:
> > On 2010/02/18 11:30:07, antonm wrote:
> > > is it true that stats_.unmapped_bytes + stats_.committed_bytes ==
> > > stats_.system_bytes?  If yes, could we either ASSERT that, or calculate
one
> > from
> > > another.
> > I haven't managed to prove this. Moreover, I've tried to add such an
assertion
> > to Check() and the tests crashed.
> > > 
> > > Plus a mix of windows language (committed) and *nix (unmapped) looks
> somewhat
> > > weird to me.
> > This is because this code was forked and existed only in the Windows
version.
> I
> > can change that if you've got a better idea.
> > 
> 
> Ditto.

Fixed. stats_.unmapped_bytes + stats_.committed_bytes == stats_.system_bytes
almost always, except for the case committed_bytes are increased temporarily in
MergeIntoFreeList. I've added several assertions in the critical points.

[Anton Muhin, 2010-03-05 15:54:35]

Thanks a lot, Alexander.

yours,
anton.

On Fri, Mar 5, 2010 at 6:25 PM,  <glider@chromium.org> wrote:
>
> http://codereview.chromium.org/576001/diff/14002/13075
> File third_party/tcmalloc/chromium/src/page_heap.cc (right):
>
> http://codereview.chromium.org/576001/diff/14002/13075#newcode283
> third_party/tcmalloc/chromium/src/page_heap.cc:283:
> stats_.unmapped_bytes += (span->length << kPageShift);
> On 2010/02/19 11:33:29, antonm wrote:
>>
>> On 2010/02/19 11:06:51, Alexander Potapenko wrote:
>> > On 2010/02/18 11:30:07, antonm wrote:
>> > > is it true that stats_.unmapped_bytes + stats_.committed_bytes ==
>> > > stats_.system_bytes?  If yes, could we either ASSERT that, or
>
> calculate one
>>
>> > from
>> > > another.
>> > I haven't managed to prove this. Moreover, I've tried to add such an
>
> assertion
>>
>> > to Check() and the tests crashed.
>> > >
>> > > Plus a mix of windows language (committed) and *nix (unmapped)
>
> looks
>>
>> somewhat
>> > > weird to me.
>> > This is because this code was forked and existed only in the Windows
>
> version.
>>
>> I
>> > can change that if you've got a better idea.
>> >
>
>> Ditto.
>
> Fixed. stats_.unmapped_bytes + stats_.committed_bytes ==
> stats_.system_bytes almost always, except for the case committed_bytes
> are increased temporarily in MergeIntoFreeList. I've added several
> assertions in the critical points.
>
> http://codereview.chromium.org/576001
>

[Alexander Potapenko, 2010-03-05 16:01:01]

On 2010/02/18 19:51:56, jar wrote:
> I see a lot of this merge relates to patching code.  I think mbelshe worked
> pretty hard to patch in a different way on Windows, and avoid the default
> patching code (which proved problematic).  Are we continuing to use the 
mbelshe
> method?
> 
Looks like the diff of patch_functions.cc is equal to the diff between that file
in perftools r76 and r86:
http://code.google.com/p/google-perftools/source/diff?spec=svn88&old=76&r=86&...

[Alexander Potapenko, 2010-03-05 17:08:53]

Gentlemen,

I've addressed all your comments (please remind me if I had missed something).
Could you please take another look at the CL and decide whether it's ready for
submission?

[antonm, 2010-03-09 17:06:01]

http://codereview.chromium.org/576001/diff/30205/29173
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/30205/29173#newcode217
third_party/tcmalloc/chromium/src/page_heap.cc:217: IncrementalScavenge(n);
Do we need to scavenge given all the spans are decommitted?

[Alexander Potapenko, 2010-03-10 13:14:24]

http://codereview.chromium.org/576001/diff/30205/29173
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/30205/29173#newcode217
third_party/tcmalloc/chromium/src/page_heap.cc:217: IncrementalScavenge(n);
On 2010/03/09 17:06:01, antonm wrote:
> Do we need to scavenge given all the spans are decommitted?

Don't see the reason not to scavenge. This is the same code we've had in
PageHeap::Delete before (some lines were just moved to PageHeap::AddToFreeList).
BTW, doesn't jar@'s comment in AddToFreeList refer to this call?

[jar (doing other things), 2010-03-10 20:00:21]

http://codereview.chromium.org/576001/diff/30205/29173
File third_party/tcmalloc/chromium/src/page_heap.cc (right):

http://codereview.chromium.org/576001/diff/30205/29173#newcode348
third_party/tcmalloc/chromium/src/page_heap.cc:348:
TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
This call looks problematic (unless something has changed more  broadly).

The release is called for a "span" which is a contiguous range of addresses. 
Sadly, spans do not need to be 64K aligned, so this can be called with
non-aligned regions spanning 64K boundaries. 

I suspect that this scavenging needs to be changed, or the page size needs to be
changed.  ...but... I'm VERY wary of a page-size change, as I'd suspect it would
notably increase memory usage.

Bottom line: I think we should not be scavenging in the Window's configuration.

Am I confused, or missing something?

[antonm, 2010-03-11 13:51:18]

lgtm

[Alexander Potapenko, 2010-03-12 15:01:53]

On 2010/03/10 20:00:21, jar wrote:
> http://codereview.chromium.org/576001/diff/30205/29173
> File third_party/tcmalloc/chromium/src/page_heap.cc (right):
> 
> http://codereview.chromium.org/576001/diff/30205/29173#newcode348
> third_party/tcmalloc/chromium/src/page_heap.cc:348:
> TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
> This call looks problematic (unless something has changed more  broadly).
> 
> The release is called for a "span" which is a contiguous range of addresses. 
> Sadly, spans do not need to be 64K aligned, so this can be called with
> non-aligned regions spanning 64K boundaries. 
> 
> I suspect that this scavenging needs to be changed, or the page size needs to
be
> changed.  ...but... I'm VERY wary of a page-size change, as I'd suspect it
would
> notably increase memory usage.
> 
> Bottom line: I think we should not be scavenging in the Window's
configuration.
> 
> Am I confused, or missing something?

Looking at the implementation of TCMalloc_SystemRelease in windows/port.cc I
suppose this is true. But then we already have this issue in the current version
of tcmalloc in Chromium, because DecommitSpan() may call TCMalloc_SystemRelease
for a span that is not properly aligned. I.e. this is a problem, but it's not
brought by the code merge.

[jar (doing other things), 2010-03-12 18:56:07]

hmm... If this is, as you say, an existing problem, it is very bad.  As a
result, I tend to think my concern is mistaken, so I went back to re-re-read
things to understand (recall?) why.

Reading the documentation on VirtualFree, I now see that we can decommit on
a page-aligned-basis, without care about the 64k boundary. I assume this
leaves us with non-64k aligned spans.  As long as we don't fully "release"
and only do "decommit," I believe we are OK.  We probably are allowed to
sub-allocate (carve) out of decommitted spans, and are allowed to
virtualAlloc (commit the carved sections) without requiring 64K alignment...
so perchance this all works out.

Sorry about my confusion (and please correct me if I'm still confused).

Jim


On Fri, Mar 12, 2010 at 7:01 AM, <glider@chromium.org> wrote:

> On 2010/03/10 20:00:21, jar wrote:
>
>> http://codereview.chromium.org/576001/diff/30205/29173
>> File third_party/tcmalloc/chromium/src/page_heap.cc (right):
>>
>
>  http://codereview.chromium.org/576001/diff/30205/29173#newcode348
>> third_party/tcmalloc/chromium/src/page_heap.cc:348:
>> TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
>> This call looks problematic (unless something has changed more  broadly).
>>
>
>  The release is called for a "span" which is a contiguous range of
>> addresses.
>> Sadly, spans do not need to be 64K aligned, so this can be called with
>> non-aligned regions spanning 64K boundaries.
>>
>
>  I suspect that this scavenging needs to be changed, or the page size needs
>> to
>>
> be
>
>> changed.  ...but... I'm VERY wary of a page-size change, as I'd suspect it
>>
> would
>
>> notably increase memory usage.
>>
>
>  Bottom line: I think we should not be scavenging in the Window's
>>
> configuration.
>
>  Am I confused, or missing something?
>>
>
> Looking at the implementation of TCMalloc_SystemRelease in windows/port.cc
> I
> suppose this is true. But then we already have this issue in the current
> version
> of tcmalloc in Chromium, because DecommitSpan() may call
> TCMalloc_SystemRelease
> for a span that is not properly aligned. I.e. this is a problem, but it's
> not
> brought by the code merge.
>
>
> http://codereview.chromium.org/576001
>

[Anton Muhin, 2010-03-12 19:04:54]

Yes, Jim, VirtualFree operates at page granularity, so we should be safe here.

yours,
anton.

On Fri, Mar 12, 2010 at 9:55 PM, Jim Roskind <jar@chromium.org> wrote:
> hmm... If this is, as you say, an existing problem, it is very bad.  As a
> result, I tend to think my concern is mistaken, so I went back to re-re-read
> things to understand (recall?) why.
> Reading the documentation on VirtualFree, I now see that we can decommit on
> a page-aligned-basis, without care about the 64k boundary. I assume this
> leaves us with non-64k aligned spans.  As long as we don't fully "release"
> and only do "decommit," I believe we are OK.  We probably are allowed to
> sub-allocate (carve) out of decommitted spans, and are allowed to
> virtualAlloc (commit the carved sections) without requiring 64K alignment...
> so perchance this all works out.
> Sorry about my confusion (and please correct me if I'm still confused).
> Jim
>
>
> On Fri, Mar 12, 2010 at 7:01 AM, <glider@chromium.org> wrote:
>>
>> On 2010/03/10 20:00:21, jar wrote:
>>>
>>> http://codereview.chromium.org/576001/diff/30205/29173
>>> File third_party/tcmalloc/chromium/src/page_heap.cc (right):
>>
>>> http://codereview.chromium.org/576001/diff/30205/29173#newcode348
>>> third_party/tcmalloc/chromium/src/page_heap.cc:348:
>>> TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
>>> This call looks problematic (unless something has changed more  broadly).
>>
>>> The release is called for a "span" which is a contiguous range of
>>> addresses.
>>> Sadly, spans do not need to be 64K aligned, so this can be called with
>>> non-aligned regions spanning 64K boundaries.
>>
>>> I suspect that this scavenging needs to be changed, or the page size
>>> needs to
>>
>> be
>>>
>>> changed.  ...but... I'm VERY wary of a page-size change, as I'd suspect
>>> it
>>
>> would
>>>
>>> notably increase memory usage.
>>
>>> Bottom line: I think we should not be scavenging in the Window's
>>
>> configuration.
>>
>>> Am I confused, or missing something?
>>
>> Looking at the implementation of TCMalloc_SystemRelease in windows/port.cc
>> I
>> suppose this is true. But then we already have this issue in the current
>> version
>> of tcmalloc in Chromium, because DecommitSpan() may call
>> TCMalloc_SystemRelease
>> for a span that is not properly aligned. I.e. this is a problem, but it's
>> not
>> brought by the code merge.
>>
>> http://codereview.chromium.org/576001
>
>

[Alexander Potapenko, 2010-03-15 05:37:03]

I'm not that familiar with Windows memory management, but MSDN confirms that
Virtual{Alloc,Free} operate page-aligned addresses.

On 2010/03/12 18:56:07, jar wrote:
> hmm... If this is, as you say, an existing problem, it is very bad.  As a
> result, I tend to think my concern is mistaken, so I went back to re-re-read
> things to understand (recall?) why.
> 
> Reading the documentation on VirtualFree, I now see that we can decommit on
> a page-aligned-basis, without care about the 64k boundary. I assume this
> leaves us with non-64k aligned spans.  As long as we don't fully "release"
> and only do "decommit," I believe we are OK.  We probably are allowed to
> sub-allocate (carve) out of decommitted spans, and are allowed to
> virtualAlloc (commit the carved sections) without requiring 64K alignment...
> so perchance this all works out.
> 
> Sorry about my confusion (and please correct me if I'm still confused).
> 
> Jim

[jar (doing other things), 2010-03-15 07:57:41]

Virtual alloc always returns (when doing an actual alloc) a 64K aligned
block.  You can get away with doing VirtualAlloc commits, and decommits with
page-level granularity.  Attempts to do actual allocations with sizes
smaller than 64K alignment will result in fragmentation of the VM space, as
consecutive allocations will skip past the insufficiently aligned fragments
so as to ONLY return 64K aligned blocks... which can be devastating to
TCMalloc, which attempts ot coalesce "spans" of allocated memory.  I believe
this problematic behavior was visible on Vista and/or XP.

This issues was a source of a major fragmentation bug in the
TCMalloc/Windows implementation around September, and the current Windows
TCMalloc implementation in Chrome is careful to increase allocator calls up
to sizes that are multiples of 64K.

You can get away with doing page-level de-commits, and commits via
VirtualAlloc... but initial allocations appear to always be 64K aligned.
(Kudos to AntonM for diagnosing the underlying cause a few months back when
JamesR and I developed the repro and plotted the fragmented allocation
pattern that resulted).

Jim

On Sun, Mar 14, 2010 at 9:37 PM, <glider@chromium.org> wrote:

> I'm not that familiar with Windows memory management, but MSDN confirms
> that
> Virtual{Alloc,Free} operate page-aligned addresses.
>
>
> On 2010/03/12 18:56:07, jar wrote:
>
>> hmm... If this is, as you say, an existing problem, it is very bad.  As a
>> result, I tend to think my concern is mistaken, so I went back to
>> re-re-read
>> things to understand (recall?) why.
>>
>
>  Reading the documentation on VirtualFree, I now see that we can decommit
>> on
>> a page-aligned-basis, without care about the 64k boundary. I assume this
>> leaves us with non-64k aligned spans.  As long as we don't fully "release"
>> and only do "decommit," I believe we are OK.  We probably are allowed to
>> sub-allocate (carve) out of decommitted spans, and are allowed to
>> virtualAlloc (commit the carved sections) without requiring 64K
>> alignment...
>> so perchance this all works out.
>>
>
>  Sorry about my confusion (and please correct me if I'm still confused).
>>
>
>  Jim
>>
>
> http://codereview.chromium.org/576001
>

[Alexander Potapenko, 2010-03-15 16:45:03]

Oh, I see. Thanks for the clarification.
> You can get away with doing page-level de-commits, and commits via
> VirtualAlloc... but initial allocations appear to always be 64K aligned.
> (Kudos to AntonM for diagnosing the underlying cause a few months back when
> JamesR and I developed the repro and plotted the fragmented allocation
> pattern that resulted).
> 
> Jim
>

[Lei Zhang, 2010-03-16 20:13:26]

http://codereview.chromium.org/576001/diff/30205/29159
File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):

http://codereview.chromium.org/576001/diff/30205/29159#newcode141
third_party/tcmalloc/chromium/src/tcmalloc.cc:141: using
tcmalloc::PageHeapAllocator;
nit: We don't use this anywhere. Same for page_heal_allocator.h above.

[Alexander Potapenko, 2010-03-17 12:33:00]

On 2010/03/16 20:13:26, Lei Zhang wrote:
> http://codereview.chromium.org/576001/diff/30205/29159
> File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):
> 
> http://codereview.chromium.org/576001/diff/30205/29159#newcode141
> third_party/tcmalloc/chromium/src/tcmalloc.cc:141: using
> tcmalloc::PageHeapAllocator;
> nit: We don't use this anywhere. Same for page_heal_allocator.h above.

Fixed.

[Alexander Potapenko, 2010-03-17 12:37:23]

On 2010/03/10 20:00:21, jar wrote:
> http://codereview.chromium.org/576001/diff/30205/29173
> File third_party/tcmalloc/chromium/src/page_heap.cc (right):
> 
> http://codereview.chromium.org/576001/diff/30205/29173#newcode348
> third_party/tcmalloc/chromium/src/page_heap.cc:348:
> TCMalloc_SystemRelease(reinterpret_cast<void*>(s->start << kPageShift),
> This call looks problematic (unless something has changed more  broadly).
> 
> The release is called for a "span" which is a contiguous range of addresses. 
> Sadly, spans do not need to be 64K aligned, so this can be called with
> non-aligned regions spanning 64K boundaries. 
> 
> I suspect that this scavenging needs to be changed, or the page size needs to
be
> changed.  ...but... I'm VERY wary of a page-size change, as I'd suspect it
would
> notably increase memory usage.
> 
> Bottom line: I think we should not be scavenging in the Window's
configuration.
> 
> Am I confused, or missing something?

Jim,

do we have any concerns on the 64k-aligned things in Windows left?

[jar (doing other things), 2010-03-17 16:34:35]

I withdraw my concerns about 64K alignment, given that we ported the basic fix
from AntonM.  I believe decommit/commit will not be impacted (but we do have to
continue to avoid Release).

Please watch all perf benchmarks very carefully when you land!

LGTM

[Alexander Potapenko, 2010-03-18 11:42:37]

I've occasionally committed the patch containing some debugging printfs.
Reverted.

On 2010/03/17 16:34:35, jar wrote:
> I withdraw my concerns about 64K alignment, given that we ported the basic fix
> from AntonM.  I believe decommit/commit will not be impacted (but we do have
to
> continue to avoid Release).
> 
> Please watch all perf benchmarks very carefully when you land!
> 
> LGTM

[Alexander Potapenko, 2010-03-18 12:19:41]

Fixed, trying to land it again.