base: Add support for malloc zones to the allocator shim

base: Add support for malloc zones to the allocator shim

This CL has no functional change, but could cause performance
regressions. It adds an additional void* context parameter to all
allocator-shim related functions, as it will be necessary to support
multiple malloc zones.

BUG=693237
TBR=thakis@chromium.org

Review-Url: https://codereview.chromium.org/2697123007
Cr-Commit-Position: refs/heads/master@{#451612}
Committed: https://chromium.googlesource.com/chromium/src/+/eff0ecbf12a6757ebb46438100fef60dff531e43

[erikchen, 2017-02-17 20:56:46]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 20:57:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 21:13:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 21:13:25]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[erikchen, 2017-02-17 21:17:22]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 21:18:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 22:36:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 22:36:02]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[erikchen, 2017-02-17 22:55:57]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 22:57:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-02-18 00:51:21]

Description was changed from

==========
Plumbing for multiple malloc zones

BUG=
==========

to

==========
macOS: Plumbing for multiple malloc zones.

This CL has no functional change, but could cause performance regressions. It
adds an additional void* context parameter to all allocator-shim related
functions, as it will be necessary to support multiple malloc zones.

BUG=693237
==========

[erikchen, 2017-02-18 00:55:56]

erikchen@chromium.org changed reviewers:
+ primiano@chromium.org

[erikchen, 2017-02-18 00:55:57]

primiano: Please review. 

If it looks good, please let this at a weird hour.

[erikchen, 2017-02-18 00:56:10]

On 2017/02/18 00:55:57, erikchen wrote:
> primiano: Please review. 
> 
> If it looks good, please let this at a weird hour.

s/let/land

[commit-bot: I haz the power, 2017-02-18 01:00:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 01:00:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[erikchen, 2017-02-18 02:23:16]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 02:24:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 04:25:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 04:25:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Primiano Tucci (use gerrit), 2017-02-20 10:24:26]

Description was changed from

==========
macOS: Plumbing for multiple malloc zones.

This CL has no functional change, but could cause performance regressions. It
adds an additional void* context parameter to all allocator-shim related
functions, as it will be necessary to support multiple malloc zones.

BUG=693237
==========

to

==========
base: Add support for malloc zones to the allocator shim

This CL has no functional change, but could cause performance regressions.
It adds an additional void* context parameter to all allocator-shim related
functions,
as it will be necessary to support multiple malloc zones.

BUG=693237
TBR=thakis@chromium.org
==========

[Primiano Tucci (use gerrit), 2017-02-20 10:31:48]

Description was changed from

==========
base: Add support for malloc zones to the allocator shim

This CL has no functional change, but could cause performance regressions.
It adds an additional void* context parameter to all allocator-shim related
functions,
as it will be necessary to support multiple malloc zones.

BUG=693237
TBR=thakis@chromium.org
==========

to

==========
base: Add support for malloc zones to the allocator shim

This CL has no functional change, but could cause performance
regressions. It adds an additional void* context parameter to all
allocator-shim related functions, as it will be necessary to support
multiple malloc zones.

BUG=693237
TBR=thakis@chromium.org
==========

[Primiano Tucci (use gerrit), 2017-02-20 10:33:05]

primiano@chromium.org changed reviewers:
+ thakis@chromium.org

[Primiano Tucci (use gerrit), 2017-02-20 10:33:05]

LGTM to get some this in the waterfall and get perf data, as Monday EMEA morning
is most quiet period of the week ever.
I would have preferred if the variable was explicitly called osx_zone or
mac_zone instead of context. context IMHO creates a shade of mystery around all
this, as "context" can mean any random thing.
anyhow, not a reason to delay this though, let's clean it up in a followup CL if
this sticks.

Slightly unrelated, by reading the code, I just realized that we might reduce
the calls to malloc_default_zone() right now we call it on each allocation,
while we could probably cache the zone ptr in the g_default_zone (which today
stores only the function pointers). Let's discuss this separately.

Also I took the liberty to:
- rewrite the 1st line of the commit message, the previous one didn't clarify
that it was touching the allocator
- wrap the rest of the commit message @ 72 cols.
- TBR thakis@ for the remaining changes in debug/thread_heap_usage_tracker.cc
which are outside of my OWNERS powerz and are just mechanically adding another
argument. (sorry for the rush Nico, but I really want this CL to go in the perf
waterfall as much isolated as possible from the rest to see if regresses any
other platform)

https://codereview.chromium.org/2697123007/diff/40001/base/allocator/allocato...
File base/allocator/allocator_shim.cc (right):

https://codereview.chromium.org/2697123007/diff/40001/base/allocator/allocato...
base/allocator/allocator_shim.cc:250: return ShimMemalign(GetCachedPageSize(),
size, nullptr);
add a comment saying that this is nullptr because pvalloc is glibc only and does
not exist on OSX/BSD systems

[Primiano Tucci (use gerrit), 2017-02-20 10:33:12]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2017-02-20 10:33:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 12:03:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 12:03:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Primiano Tucci (use gerrit), 2017-02-20 12:37:05]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2017-02-20 12:37:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 13:04:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487594225712470,
"parent_rev": "e5399d9f871fab1ea1e7b061125106199ee0545a", "commit_rev":
"eff0ecbf12a6757ebb46438100fef60dff531e43"}

[commit-bot: I haz the power, 2017-02-20 13:05:27]

Description was changed from

==========
base: Add support for malloc zones to the allocator shim

This CL has no functional change, but could cause performance
regressions. It adds an additional void* context parameter to all
allocator-shim related functions, as it will be necessary to support
multiple malloc zones.

BUG=693237
TBR=thakis@chromium.org
==========

to

==========
base: Add support for malloc zones to the allocator shim

This CL has no functional change, but could cause performance
regressions. It adds an additional void* context parameter to all
allocator-shim related functions, as it will be necessary to support
multiple malloc zones.

BUG=693237
TBR=thakis@chromium.org

Review-Url: https://codereview.chromium.org/2697123007
Cr-Commit-Position: refs/heads/master@{#451612}
Committed:
https://chromium.googlesource.com/chromium/src/+/eff0ecbf12a6757ebb46438100fe...
==========

[commit-bot: I haz the power, 2017-02-20 13:05:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/eff0ecbf12a6757ebb46438100fe...

[erikchen, 2017-02-21 19:20:43]

On 2017/02/20 10:33:05, Primiano Tucci wrote:
> LGTM to get some this in the waterfall and get perf data, as Monday EMEA
morning
> is most quiet period of the week ever.
> I would have preferred if the variable was explicitly called osx_zone or
> mac_zone instead of context. context IMHO creates a shade of mystery around
all
> this, as "context" can mean any random thing.
> anyhow, not a reason to delay this though, let's clean it up in a followup CL
if
> this sticks.
I intentionally named this "context" because there is a non-zero probability
that we will need something similar for Windows if we want to shim HeapAlloc.

> 
> Slightly unrelated, by reading the code, I just realized that we might reduce
> the calls to malloc_default_zone() right now we call it on each allocation,
> while we could probably cache the zone ptr in the g_default_zone (which today
> stores only the function pointers). Let's discuss this separately.
> 
> Also I took the liberty to:
> - rewrite the 1st line of the commit message, the previous one didn't clarify
> that it was touching the allocator
> - wrap the rest of the commit message @ 72 cols.
> - TBR thakis@ for the remaining changes in debug/thread_heap_usage_tracker.cc
> which are outside of my OWNERS powerz and are just mechanically adding another
> argument. (sorry for the rush Nico, but I really want this CL to go in the
perf
> waterfall as much isolated as possible from the rest to see if regresses any
> other platform)
> 
>
https://codereview.chromium.org/2697123007/diff/40001/base/allocator/allocato...
> File base/allocator/allocator_shim.cc (right):
> 
>
https://codereview.chromium.org/2697123007/diff/40001/base/allocator/allocato...
> base/allocator/allocator_shim.cc:250: return ShimMemalign(GetCachedPageSize(),
> size, nullptr);
> add a comment saying that this is nullptr because pvalloc is glibc only and
does
> not exist on OSX/BSD systems