Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1613183002:
allocator: Remove tcmalloc override when build_for_tool==memcheck (Closed)
|
Created:
4 years, 11 months ago by Primiano Tucci (use gerrit) Modified:
4 years, 11 months ago CC:
chromium-reviews, oshima Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionallocator: Remove tcmalloc override when build_for_tool==memcheck
Practical reason of this CL:
This is causing a breakage in the Mac Valgrind builder
after crrev.com/1616793003:
https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Builder%20%28valgrind%29/builds/91232/steps/runhooks/logs/stdio
Longer explanation:
crrev.com/1616793003 made allocator target toolset: host, target
(which itself should be OK)
A very old condition was forcing use_allocator=tcmalloc on all OS
when build_for_tool=memcheck.
This causes problems now on the Mac valgrind bot when building the
chromium_builder_dbg_valgrind_mac target.
In the last change that touched this line, crrev.com/258433005, the
original condition was linux_use_tcmalloc=1. This was mistakenly
translated in a blanket allocator==tcmalloc.
Rationale for removing this line:
allocator=tcmalloc is the default on linux these days. I don't see a
reason why re-enforcing this condition, as use_allocator=tcmalloc by
default on the right (Linux/Android) platforms.
BUG=564618, 580108
TEST=gyp_chromium -Dbuild_for_tool=memcheck && ninja chromium_builder_dbg_valgrind_mac
Committed: https://crrev.com/bb9da71e07aa9e8af0745f8866c72570d8187ac4
Cr-Commit-Position: refs/heads/master@{#370724}
Patch Set 1 #
Messages
Total messages: 17 (9 generated)
Description was changed from ========== allocator: Remove tcmalloc override when build_for_tool==memcheck This is causing a breakage in the Mac Valgrind builder. https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... BUG=564618 ========== to ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc unconditionally (on all OS, including Mac) when build_for_tool=memcheck. This caused problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618 ==========
Description was changed from ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc unconditionally (on all OS, including Mac) when build_for_tool=memcheck. This caused problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618 ========== to ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618 ==========
primiano@chromium.org changed reviewers: + rnk@chromium.org, thakis@chromium.org
Description was changed from ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618 ========== to ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618,580108 ==========
For the record I did reproduce the breakage locally and confirm that this patch fix fixes it.
Description was changed from ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618,580108 ========== to ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618,580108 TEST=gyp_chromium -Dbuild_for_tool=memcheck && ninja chromium_builder_dbg_valgrind_mac ==========
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
lgtm
(assuming no equivalent GN change required?)
On 2016/01/21 17:29:35, scottmg wrote: > (assuming no equivalent GN change required?) Didn't find any traces of build_for_tool or memcheck in gn files. I assume this is not a thing there?
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1613183002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1613183002/1
Message was sent while issue was closed.
Description was changed from ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618,580108 TEST=gyp_chromium -Dbuild_for_tool=memcheck && ninja chromium_builder_dbg_valgrind_mac ========== to ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618,580108 TEST=gyp_chromium -Dbuild_for_tool=memcheck && ninja chromium_builder_dbg_valgrind_mac ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618,580108 TEST=gyp_chromium -Dbuild_for_tool=memcheck && ninja chromium_builder_dbg_valgrind_mac ========== to ========== allocator: Remove tcmalloc override when build_for_tool==memcheck Practical reason of this CL: This is causing a breakage in the Mac Valgrind builder after crrev.com/1616793003: https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20Bu... Longer explanation: crrev.com/1616793003 made allocator target toolset: host, target (which itself should be OK) A very old condition was forcing use_allocator=tcmalloc on all OS when build_for_tool=memcheck. This causes problems now on the Mac valgrind bot when building the chromium_builder_dbg_valgrind_mac target. In the last change that touched this line, crrev.com/258433005, the original condition was linux_use_tcmalloc=1. This was mistakenly translated in a blanket allocator==tcmalloc. Rationale for removing this line: allocator=tcmalloc is the default on linux these days. I don't see a reason why re-enforcing this condition, as use_allocator=tcmalloc by default on the right (Linux/Android) platforms. BUG=564618,580108 TEST=gyp_chromium -Dbuild_for_tool=memcheck && ninja chromium_builder_dbg_valgrind_mac Committed: https://crrev.com/bb9da71e07aa9e8af0745f8866c72570d8187ac4 Cr-Commit-Position: refs/heads/master@{#370724} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/bb9da71e07aa9e8af0745f8866c72570d8187ac4 Cr-Commit-Position: refs/heads/master@{#370724} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
