Reorder checks in tcmalloc.

Reorder checks in tcmalloc.

One of our local changes to tcmalloc is logic to reject all
allocations above a certain size. (At least those via malloc and so
on.)  The check for that doesn't need to be on the hot path.  Most
allocations are small and there is a special case for that already.
If that special case applies then we don't need to check if the
allocation is gigantic.

This is a performance tuning change.  It doesn't affect functionality.

To evaluate this change I ran 3 page cyclers: bloat, dhtml, and
morejs.  I used ChromeOS on the Samsung 550.  I ran 30 trials; a few
randomly failed, but I got close to 30 "before" and "after" results
for each of those 3 tests.

Results in order of p-value were:
Bloat  0.3% faster, p=0.02
DHTML  0.3% faster, p=0.46
MoreJS 0.1% faster, p=0.82

This looks like a useful gain, but it's small, as one would expect.

BTW I'm trusting the compiler to rewrite
(x < (constant0)) && (x < (constant1))
to
(x < (the smaller of the two constants))

I checked the generated code, and it looks good (at least on the
ChromeOS toolchain for Samsung 550).

BUG=111726
TEST=none
R=jar


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189321

[jar (doing other things), 2013-03-15 23:00:47]

nit suggestion and comment below.

https://codereview.chromium.org/12572005/diff/2001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):

https://codereview.chromium.org/12572005/diff/2001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/tcmalloc.cc:1092: if (size <= kMaxSize &&
IsAllocSizePermitted(size)) {
It would probably be clearer to not even risk calling IsAllocSizePermitted()
here, and instead put a DEBUG assertion

ASSERT(IsAllocSizePermitted(size)); 

in front of line 1093.  This way you don't have to trust the compiler (and the
meaning of the code is clearer).

https://codereview.chromium.org/12572005/diff/2001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/tcmalloc.cc:1100:
heap->AddToByteAllocatedTotal(size);  // Chromium profiling.
...but this line of optimization points a scary finger at me here.  Is this
costly, or is it free, since we have the struct in memory via a cache line? 
Inquiring minds want to know.  If it is not expensive, you could clobber my
paranoid comment (inserted when I added the instrumentation).

[jln (very slow on Chromium), 2013-03-17 10:21:51]

https://codereview.chromium.org/12572005/diff/2001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):

https://codereview.chromium.org/12572005/diff/2001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/tcmalloc.cc:1092: if (size <= kMaxSize &&
IsAllocSizePermitted(size)) {
On 2013/03/15 23:00:47, jar wrote:
> It would probably be clearer to not even risk calling IsAllocSizePermitted()
> here, and instead put a DEBUG assertion
> 
> ASSERT(IsAllocSizePermitted(size)); 
> 
> in front of line 1093.  This way you don't have to trust the compiler (and the
> meaning of the code is clearer).

For security, I would really be wary of making anything debug only. This check
is useful when an attacker is trying to "trick" something into allocating a
large object to exploit vulnerabilities such as the prevalent "int instead of
size_t" on 64 bits Chrome.
Making it a debug check won't work for security.

I would rather leave the check and hope that the compiler makes the right
decision, and if it doesn't, have a small perf impact rather than a security
vulnerability.

If you wish, we could instead have a DCHECK inside IsAllocSizePermitted()
instead, for size > kMaxSize, with an explaining comment.

[gpike, 2013-03-19 21:55:11]

I put an ASSERT at the bottom of do_malloc() that may help clarify the intent. 
I don't think we should worry about the compiler.  It's easy for a compiler to
know that "x < 3 && x < 4" is equivalent to "x < 3."  Feel free to overrule me,
though.

Jim, I looked at your TODO.  Your bytes-allocated counter gets updated by a
single instruction (on x86).  Some other stuff nearby gets reordered a bit, but
I don't think it really matters.  (The reason the change I'm proposing here
makes more difference is that removes a conditional branch in the common case. 
Branches are more expensive on average than "go add a number to some field of a
data structure.")  So, I removed the TODO.  Optimistically.  The timing results
will be available tonight.

[jar (doing other things), 2013-03-19 21:59:57]

LGTM

[jln (very slow on Chromium), 2013-03-19 22:04:12]

lgtm

[gpike, 2013-03-20 07:37:24]

Regarding that bytes-allocated counter: I ran 30 trials "with it" and 30
"without it" in an experiment like the one described above, but this time on an
Acer C7 Chromebook. None of the benchmarks I tried (BloatFile, DhtmlFile,
MoreJSFile) had a significant different; p values were .47 and higher.

[commit-bot: I haz the power, 2013-03-20 07:37:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gpike@chromium.org/12572005/7001

[commit-bot: I haz the power, 2013-03-20 10:37:30]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[jar (doing other things), 2013-03-20 16:04:16]

Thanks for checking it out!!!

On 2013/03/20 07:37:24, gpike wrote:
> Regarding that bytes-allocated counter: I ran 30 trials "with it" and 30
> "without it" in an experiment like the one described above, but this time on
an
> Acer C7 Chromebook. None of the benchmarks I tried (BloatFile, DhtmlFile,
> MoreJSFile) had a significant different; p values were .47 and higher.

[commit-bot: I haz the power, 2013-03-20 16:26:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gpike@chromium.org/12572005/7001

[commit-bot: I haz the power, 2013-03-20 17:11:42]

Change committed as 189321