Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640263003:
tcmalloc: Use the default TLS model on arm-gcc, CrOS only. (Closed)
|
Created:
3 years, 11 months ago by Ting-Yuan (Leo) Huang Modified:
3 years, 10 months ago Reviewers:
Primiano Tucci (use gerrit), Alexander Potapenko, chihchung, laszio, llozano, Will Harris CC:
chromium-reviews, Dai Mikurube (NOT FULLTIME) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontcmalloc: Use the default TLS model on arm-gcc, CrOS only.
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
BUG=682840
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
Review-Url: https://codereview.chromium.org/2640263003
Cr-Commit-Position: refs/heads/master@{#445109}
Committed: https://chromium.googlesource.com/chromium/src/+/07dcf42f46e211a53f7c33ed634cc5f416a3929f
Patch Set 1 #
Total comments: 3
Patch Set 2 : tcmalloc: Use the default TLS model on arm-gcc #
Total comments: 3
Patch Set 3 : tcmalloc: Use the default TLS model on arm-gcc, CrOS only. #
Messages
Total messages: 33 (17 generated)
Description was changed from
==========
tcmalloc: Use the default TLS model on arm-gcc
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
See crbug.com/629593 and crbug.com/650137 for more details.
BUG=629593
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
==========
to
==========
tcmalloc: Use the default TLS model on arm-gcc
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
See crbug.com/629593 and crbug.com/650137 for more details.
BUG=629593
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
==========
laszio@google.com changed reviewers: + primiano@chromium.org, wfh@chromium.org
Although Chih-Chung's solution is strictly better, it requires substantially more work and may miss the R56 release: https://codereview.chromium.org/2363283002/ This CL is still a net win. The change only applies to chromeos on arm by gcc which suffers from the TLS bug. It is also simple and should be pretty safe. The performance gain on page_cycler_v2.typical_25 is around +8.5%. Shall we land this first and rebase Chih-Chung's patch once it is finished?
llozano@chromium.org changed reviewers: + llozano@chromium.org
https://codereview.chromium.org/2640263003/diff/1/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/thread_cache.cc (right): https://codereview.chromium.org/2640263003/diff/1/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/thread_cache.cc:73: (defined(__clang__) || !defined(OS_CHROMEOS) || !defined(__arm__)) not gcc does not necessarily mean clang. Is it? I prefer to read this condition like !(GCC && OS_CHROMEOS && ARM)
On 2017/01/19 21:38:42, llozano wrote: > https://codereview.chromium.org/2640263003/diff/1/third_party/tcmalloc/chromi... > File third_party/tcmalloc/chromium/src/thread_cache.cc (right): > > https://codereview.chromium.org/2640263003/diff/1/third_party/tcmalloc/chromi... > third_party/tcmalloc/chromium/src/thread_cache.cc:73: (defined(__clang__) || > !defined(OS_CHROMEOS) || !defined(__arm__)) > not gcc does not necessarily mean clang. Is it? > > I prefer to read this condition like > !(GCC && OS_CHROMEOS && ARM) also, I think we should create a new BUG. The one you are using is related but it is not what we are fixing here. make the bug a P1 or P0 to show the importance of getting this reviewed and merged ASAP. please mention in description that we recover an 8% in performance with this fix.
https://codereview.chromium.org/2640263003/diff/1/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/thread_cache.cc (right): https://codereview.chromium.org/2640263003/diff/1/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/thread_cache.cc:73: (defined(__clang__) || !defined(OS_CHROMEOS) || !defined(__arm__)) On 2017/01/19 21:38:42, llozano wrote: > not gcc does not necessarily mean clang. Is it? > > I prefer to read this condition like > !(GCC && OS_CHROMEOS && ARM) > ah, but on ChromeOS .. not GCC implies clang..
laszio@google.com changed reviewers: + laszio@google.com
Created a new bug on crbug.com/682840. https://codereview.chromium.org/2640263003/diff/1/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/thread_cache.cc (right): https://codereview.chromium.org/2640263003/diff/1/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/thread_cache.cc:73: (defined(__clang__) || !defined(OS_CHROMEOS) || !defined(__arm__)) On 2017/01/19 21:38:42, llozano wrote: > not gcc does not necessarily mean clang. Is it? > > I prefer to read this condition like > !(GCC && OS_CHROMEOS && ARM) > Done.
laszio@google.com changed reviewers: + chihchung@chromium.org
laszio@google.com changed reviewers: + glider@chromium.org
Description was changed from
==========
tcmalloc: Use the default TLS model on arm-gcc
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
See crbug.com/629593 and crbug.com/650137 for more details.
BUG=629593
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
==========
to
==========
tcmalloc: Use the default TLS model on arm-gcc
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
BUG=682840
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
==========
The CQ bit was checked by laszio@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'd like to defer this review to primiano as I'm not too familiar with this part of the tcmalloc code.
On 2017/01/19 23:30:04, Will Harris wrote: > I'd like to defer this review to primiano as I'm not too familiar with this part > of the tcmalloc code. Happy to review tomorrow as I get in the office (UK). Just one question: how come this one doesn't have the same problems of the other change w.r.t. extra .so libs? What changed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/19 23:32:22, primiano CORP (USE chromium) wrote: > On 2017/01/19 23:30:04, Will Harris wrote: > > I'd like to defer this review to primiano as I'm not too familiar with this > part > > of the tcmalloc code. > > Happy to review tomorrow as I get in the office (UK). > Just one question: how come this one doesn't have the same problems of the other > change w.r.t. extra .so libs? What changed? chihchung was changing the tls model to "local-exec", which cannot be used within a shared library. So, it seems the linker complains abouts This change sets the tls model value to default which is less restrictive and less performing than local-exec but fixes the problem in GCC. see: https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Code-Gen-Options.html look for "local-exec" (we always build with -fpic).
LGTM.
LGTM with one comment Also could you please update the cl description to mention something like "CrOS only" in the title (first line)? https://codereview.chromium.org/2640263003/diff/20001/third_party/tcmalloc/ch... File third_party/tcmalloc/chromium/src/thread_cache.cc (right): https://codereview.chromium.org/2640263003/diff/20001/third_party/tcmalloc/ch... third_party/tcmalloc/chromium/src/thread_cache.cc:73: !(!defined(__clang__) && defined(OS_CHROMEOS) && defined(__arm__)) ditto https://codereview.chromium.org/2640263003/diff/20001/third_party/tcmalloc/ch... File third_party/tcmalloc/chromium/src/thread_cache.h (right): https://codereview.chromium.org/2640263003/diff/20001/third_party/tcmalloc/ch... third_party/tcmalloc/chromium/src/thread_cache.h:275: !(!defined(__clang__) && defined(OS_CHROMEOS) && defined(__arm__)) why !(!defined(__clang__)) rather than just defined(__clang__) ?
https://codereview.chromium.org/2640263003/diff/20001/third_party/tcmalloc/ch... File third_party/tcmalloc/chromium/src/thread_cache.h (right): https://codereview.chromium.org/2640263003/diff/20001/third_party/tcmalloc/ch... third_party/tcmalloc/chromium/src/thread_cache.h:275: !(!defined(__clang__) && defined(OS_CHROMEOS) && defined(__arm__)) On 2017/01/20 14:31:04, Primiano Tucci wrote: > why !(!defined(__clang__)) rather than just defined(__clang__) ? It's in a group that belongs to the new condition that we want to exclude: !( !defined(__clang__) && defined(OS_CHROMEOS) && defined(__arm__) ) Because __GNUC__ is also defined by clang, __clang__ seems to be the simplest way to distinguish them, at the cost of the looking of the code.
Description was changed from
==========
tcmalloc: Use the default TLS model on arm-gcc
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
BUG=682840
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
==========
to
==========
tcmalloc: Use the default TLS model on arm-gcc, CrOS only.
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
BUG=682840
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
==========
The CQ bit was checked by laszio@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, chihchung@chromium.org Link to the patchset: https://codereview.chromium.org/2640263003/#ps40001 (title: "tcmalloc: Use the default TLS model on arm-gcc, CrOS only.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/20 17:56:12, laszio wrote: > https://codereview.chromium.org/2640263003/diff/20001/third_party/tcmalloc/ch... > File third_party/tcmalloc/chromium/src/thread_cache.h (right): > > https://codereview.chromium.org/2640263003/diff/20001/third_party/tcmalloc/ch... > third_party/tcmalloc/chromium/src/thread_cache.h:275: !(!defined(__clang__) && > defined(OS_CHROMEOS) && defined(__arm__)) > On 2017/01/20 14:31:04, Primiano Tucci wrote: > > why !(!defined(__clang__)) rather than just defined(__clang__) ? > > It's in a group that belongs to the new condition that we want to exclude: > !( !defined(__clang__) && defined(OS_CHROMEOS) && defined(__arm__) ) > > Because __GNUC__ is also defined by clang, __clang__ seems to be the simplest > way to distinguish them, at the cost of the looking of the code. Oh, ignore what I said. I miscounted the parentheses. For a moment I thought you wrote !(!defined(__clang_))
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484935176022110,
"parent_rev": "004f5aa5fdefd9677e0fe9c7bdd4467525c5f2ae", "commit_rev":
"07dcf42f46e211a53f7c33ed634cc5f416a3929f"}
Message was sent while issue was closed.
Description was changed from
==========
tcmalloc: Use the default TLS model on arm-gcc, CrOS only.
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
BUG=682840
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
==========
to
==========
tcmalloc: Use the default TLS model on arm-gcc, CrOS only.
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
BUG=682840
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
Review-Url: https://codereview.chromium.org/2640263003
Cr-Commit-Position: refs/heads/master@{#445109}
Committed:
https://chromium.googlesource.com/chromium/src/+/07dcf42f46e211a53f7c33ed634c...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/07dcf42f46e211a53f7c33ed634c...
Message was sent while issue was closed.
Hallo wfh@chromium.org! Due to a depot_tools patch which mistakenly removed the OWNERS check for non-source (and some other filtered) files (see crbug.com/684270), the following files landed in this CL and need a retrospective review from you: third_party/tcmalloc/README.chromium third_party/tcmalloc/chromium/src/thread_cache.cc third_party/tcmalloc/chromium/src/thread_cache.h Thanks, Wez
Message was sent while issue was closed.
Description was changed from
==========
tcmalloc: Use the default TLS model on arm-gcc, CrOS only.
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
BUG=682840
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
Review-Url: https://codereview.chromium.org/2640263003
Cr-Commit-Position: refs/heads/master@{#445109}
Committed:
https://chromium.googlesource.com/chromium/src/+/07dcf42f46e211a53f7c33ed634c...
==========
to
==========
tcmalloc: Use the default TLS model on arm-gcc, CrOS only.
gcc has a bug in __attribute__ ((tls_model ("initial-exec"))) on arm. This
patch undoes the TLS optimization on arm-gcc.
BUG=682840
TEST=threadlocal_heap_ works as expected; CreateCacheIfNecessary() was only
called a few times in a reasonably long benchmark.
Review-Url: https://codereview.chromium.org/2640263003
Cr-Commit-Position: refs/heads/master@{#445109}
Committed:
https://chromium.googlesource.com/chromium/src/+/07dcf42f46e211a53f7c33ed634c...
==========
lgtm from primiano is as good as lgtm from wfh |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
