Speed up tcmalloc by allowing more inlining.

Speed up tcmalloc by allowing more inlining.

Move some small functions in free_list.cc to free_list.h
to improve the chance of intelligent inlining decisions.

No functional changes.  This is a performance tuning change.

From the ChromeOS point of view, this is clearly a good change.
I didn't try it on Windows, but I suspect it will help there too.
In any case, it shouldn't hurt.

Preliminary results for the BloatFile benchmark, for example, are around a 3.2% speedup (averaged across a few runs on different platforms).  I am collecting more data now.

BUG=111726
TEST=none


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

[jar (doing other things), 2013-03-14 00:05:09]

Nice!

Do you have a profile or heat map that identified these functions?  I'd be very
curious to see the justification that sent you down this specific path.  I've
had it on my (infinite) todo list to look at a number of things in TCMalloc, so
it is nice to see your work.

Do you know how much this increased the size of the binary?  I think that the
methods you selected are only used by the TCMalloc code (not called directly by
user code), so I *think* the impact on size will be small.  ...but inquiring
minds want to know ;-).

Since you're playing around in this area... I had one small thought listed below
(which may well be ignored, since I can't justify it in advance from data.. and
dynamic branch prediction may dwarf any compile-time optimizations).  It would
be really nice if we understood how the how the heat was distributed in these
functions.

In any case LGTM.

Please be cautious to watch for perf regressions when you land.  It might (for
example) be the case that your benchmark helps when purely allocation code is
exercised, and then there is a loss (due to lack of code locality?) when this is
called intermittently during Chromium runs.

I'd also be curious to understand if *parts* of your change dominated the
results.  For instance, the FL_Previous_No_Check() and FL_Next_No_Check() could
be inlined within *only* the cc file, and *that* might be the bulk of the 
savings!?!

https://codereview.chromium.org/12619004/diff/1/third_party/tcmalloc/chromium...
File third_party/tcmalloc/chromium/src/free_list.h (right):

https://codereview.chromium.org/12619004/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/free_list.h:65: if (v0 != v1) Log(kCrash,
file, line, "Memory corruption detected.");
Is code optimized (by default) for the true case?  If so, you may as well:
if (v0 == v1) return;

https://codereview.chromium.org/12619004/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/free_list.h:148: if (old == NULL) { // Builds
singleton list.
In most cases, I'd expect that our lists are non-null... and again, if compilers
optimize for the true cases, then we should swap this as well.

A profile would probably show what is going on (in the non-inlined case).

[commit-bot: I haz the power, 2013-03-14 00:57:45]

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

[gpike, 2013-03-14 00:58:37]

Thanks!

https://codereview.chromium.org/12619004/diff/1/third_party/tcmalloc/chromium...
File third_party/tcmalloc/chromium/src/free_list.h (right):

https://codereview.chromium.org/12619004/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/free_list.h:65: if (v0 != v1) Log(kCrash,
file, line, "Memory corruption detected.");
On 2013/03/14 00:05:09, jar wrote:
> Is code optimized (by default) for the true case?  If so, you may as well:
> if (v0 == v1) return;

I looked at the generated code and it seems fine. Also, as you say, the CPU's
branch predictor will generally do the right thing.

https://codereview.chromium.org/12619004/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/free_list.h:148: if (old == NULL) { // Builds
singleton list.
On 2013/03/14 00:05:09, jar wrote:
> In most cases, I'd expect that our lists are non-null... and again, if
compilers
> optimize for the true cases, then we should swap this as well.
> 
> A profile would probably show what is going on (in the non-inlined case).

Within the next few days I want to look at this function again, because we
"know" that element is non-NULL here, so I think we can safely change
FL_SetPrevious(element, NULL) to "ASSERT(element != NULL);
FL_SetPreviousNoCheck(element, NULL);" Although perhaps the compiler already
does that. Anyway, at that point I'll check on the point you raise.

[commit-bot: I haz the power, 2013-03-14 14:16:12]

Change committed as 188063