Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(38)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1523403003: Lock tcmalloc structures in a pthread_atfork hook.

Created:
5 years ago by rickyz (no longer on Chrome)
Modified:
5 years ago
Reviewers:
Will Harris
CC:
chromium-reviews, Dai Mikurube (NOT FULLTIME)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Lock tcmalloc structures in a pthread_atfork hook. This isn't needed in chromium, since we do not allow forking from multithreaded processes. Other embedders of chromium have requested this though (see bug). This is a modified backport of upstream commits: https://github.com/gperftools/gperftools/commit/bd3b3a7e9a67fac846cf574f5bfd241157bdfe3c https://github.com/gperftools/gperftools/commit/805a6601939edd9bca60a8911e56b217e477c75e https://github.com/gperftools/gperftools/commit/3e9a33e8c708ccf3ec91e3a3b14e924f5f79e4a6 https://github.com/gperftools/gperftools/commit/7013b219970a329d1db58fbd7fa7c907bec8dbba BUG=570068

Patch Set 1 #

Patch Set 2 : Hacks #

Total comments: 1

Patch Set 3 : Fix typo. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+48 lines, -2 lines) Patch
M third_party/tcmalloc/chromium/src/central_freelist.h View 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/tcmalloc/chromium/src/libc_override_osx.h View 1 2 chunks +7 lines, -2 lines 0 comments Download
M third_party/tcmalloc/chromium/src/static_vars.cc View 1 2 3 chunks +31 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (3 generated)
rickyz (no longer on Chrome)
5 years ago (2015-12-16 12:13:39 UTC) #4 
This is roughly what it would take to support the atfork stuff in our copy of
tcmalloc. The details ended up being a little more subtle than expected since it
turns out that component builds can have multiple copies of tcmalloc code in
libraries + the main binary (I started looking into fixing this, but not sure
how deep it goes yet).

I don't feel super strongly either way about whether it's worth adding this
additional divergence from upstream tcmalloc. Perhaps we should have a more
solid plan about how to keep up with tcmalloc before doing changes like this?

https://codereview.chromium.org/1523403003/diff/20001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/static_vars.cc (right):

https://codereview.chromium.org/1523403003/diff/20001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/static_vars.cc:90:
pthread_atfork(CentralCacheLockAll,    // parent calls before fork
Unlike the upstream commit, we keep the pthread_atfork call in InitStaticVars.
This is because in component builds, we sometimes have multiple copies of
tcmalloc in the same binary, so a module initializer can call install duplicate
hooks.

This will likely fail horribly if there are already 48 fork handlers installed,
since in that case, libpthread will need allocate memory from the heap.

We should fix the multiple copies of tcmalloc in component builds and consider
moving this back to a module initializer.

Powered by Google App Engine
This is Rietveld 408576698