Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
842673002:
[tcmalloc]: Remove calls to __system_property_get in tcmalloc. (Closed)
Description[tcmalloc]: Remove calls to __system_property_get in tcmalloc.
Removes calls to __system_property_get in tcmalloc since this is not a
public API and has been removed from the 'L' Android NDK. Since Android does
not use tcmalloc by default, this will only affect builds which explicitly
configure use_allocator=tcmalloc and profiling=1 (which is likely only used
when building for deep memory profiler).
BUG=394841
Patch Set 1 #
Total comments: 2
Messages
Total messages: 9 (2 generated)
rmcilroy@chromium.org changed reviewers: + jar@chromium.org
Jar: could you please take a look, thanks. As mentioned in the description, tcmalloc is not the default allocator on Android so this shouldn't have much impact, except for people who use the Deep Memory Profiler. I've cced you on an email which I have sent to the dmprof@ mailing list which explains the consequence of this change.
jar@chromium.org changed reviewers: + cpu@chromium.org
https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right): https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... third_party/tcmalloc/chromium/src/base/commandlineflags.h:118: #error TCMalloc does not support profiling on Android I'm missing the connection to TCMalloc. You are removing calls to __system_property_get(), presumably because it may not be available at some point. You have changed the code to not compile this file for Android (in fact, you can't compile it on Android). Can you clarify the connection to TCMalloc, and explain why blocking compilation of this file on Android is a good thing?
https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right): https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... third_party/tcmalloc/chromium/src/base/commandlineflags.h:118: #error TCMalloc does not support profiling on Android On 2015/01/07 19:39:57, jar (doing other things) wrote: > I'm missing the connection to TCMalloc. > > You are removing calls to __system_property_get(), presumably because it may not > be available at some point. > > You have changed the code to not compile this file for Android (in fact, you > can't compile it on Android). > > Can you clarify the connection to TCMalloc, and explain why blocking compilation > of this file on Android is a good thing? <DOH!> I misread the file name (I stopped looking up the path after base... src... chromium (which is about where I have the root of my tree!?!). I now see it was under third_party/tcmalloc. One obvious question: If Android is not using TCMalloc, why is this even trying to get compiled? If it is not, why are you conditioning to preclude compilation at this point?? Why not pull all mentions of Android? .... and if you want it to "sort of compile" (?) then why are you putting in a #error?
On 2015/01/07 20:28:21, jar (doing other things) wrote: > https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... > File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right): > > https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... > third_party/tcmalloc/chromium/src/base/commandlineflags.h:118: #error TCMalloc > does not support profiling on Android > On 2015/01/07 19:39:57, jar (doing other things) wrote: > > I'm missing the connection to TCMalloc. > > > > You are removing calls to __system_property_get(), presumably because it may > not > > be available at some point. > > > > You have changed the code to not compile this file for Android (in fact, you > > can't compile it on Android). > > > > Can you clarify the connection to TCMalloc, and explain why blocking > compilation > > of this file on Android is a good thing? > > <DOH!> I misread the file name (I stopped looking up the path after base... > src... chromium (which is about where I have the root of my tree!?!). I now see > it was under third_party/tcmalloc. > > One obvious question: If Android is not using TCMalloc, why is this even trying > to get compiled? If it is not, why are you conditioning to preclude compilation > at this point?? Why not pull all mentions of Android? .... and if you want it > to "sort of compile" (?) then why are you putting in a #error? FYI :) this was all added quite a while ago: https://chromiumcodereview.appspot.com/14645016 back then, myself and dmikirube needed to run deep-memory-profiler on android, which required TCMalloc. on android, we never used TCMalloc in any other scenario (production, debug, etc...), except when explicitly building for DMProf. I suppose we could try to revert that entire patch by now that there are better memory profiling tools in place (and no plans for TCMalloc)? there were a few other ones too if we want to pull all android-related changes that were made at that time..
On 2015/01/07 20:42:16, bulach wrote: > On 2015/01/07 20:28:21, jar (doing other things) wrote: > > > https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... > > File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right): > > > > > https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... > > third_party/tcmalloc/chromium/src/base/commandlineflags.h:118: #error TCMalloc > > does not support profiling on Android > > On 2015/01/07 19:39:57, jar (doing other things) wrote: > > > I'm missing the connection to TCMalloc. > > > > > > You are removing calls to __system_property_get(), presumably because it may > > not > > > be available at some point. > > > > > > You have changed the code to not compile this file for Android (in fact, you > > > can't compile it on Android). > > > > > > Can you clarify the connection to TCMalloc, and explain why blocking > > compilation > > > of this file on Android is a good thing? > > > > <DOH!> I misread the file name (I stopped looking up the path after base... > > src... chromium (which is about where I have the root of my tree!?!). I now > see > > it was under third_party/tcmalloc. > > > > One obvious question: If Android is not using TCMalloc, why is this even > trying > > to get compiled? If it is not, why are you conditioning to preclude > compilation > > at this point?? Why not pull all mentions of Android? .... and if you want > it > > to "sort of compile" (?) then why are you putting in a #error? > > FYI :) > this was all added quite a while ago: > https://chromiumcodereview.appspot.com/14645016 > > back then, myself and dmikirube needed to run deep-memory-profiler on android, > which required TCMalloc. > on android, we never used TCMalloc in any other scenario (production, debug, > etc...), except when explicitly building for DMProf. > I suppose we could try to revert that entire patch by now that there are better > memory profiling tools in place (and no plans for TCMalloc)? > > there were a few other ones too if we want to pull all android-related changes > that were made at that time.. The code you linked to appears to have more than just Android porting code.... but I would argue that IF you are removing Android support for TCMalloc, you should clean the code more thoroughly, and remove the resulting (now) dead code. At a minimum, any mention of "android" (in #if, etc.) should be removed. It is too easy to just leave dead code around.... and make the codebase progressively less readable.
On 2015/01/07 21:53:36, jar (doing other things) wrote: > On 2015/01/07 20:42:16, bulach wrote: > > On 2015/01/07 20:28:21, jar (doing other things) wrote: > > > > > > https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... > > > File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right): > > > > > > > > > https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... > > > third_party/tcmalloc/chromium/src/base/commandlineflags.h:118: #error > TCMalloc > > > does not support profiling on Android > > > On 2015/01/07 19:39:57, jar (doing other things) wrote: > > > > I'm missing the connection to TCMalloc. > > > > > > > > You are removing calls to __system_property_get(), presumably because it > may > > > not > > > > be available at some point. > > > > > > > > You have changed the code to not compile this file for Android (in fact, > you > > > > can't compile it on Android). > > > > > > > > Can you clarify the connection to TCMalloc, and explain why blocking > > > compilation > > > > of this file on Android is a good thing? > > > > > > <DOH!> I misread the file name (I stopped looking up the path after base... > > > src... chromium (which is about where I have the root of my tree!?!). I now > > see > > > it was under third_party/tcmalloc. > > > > > > One obvious question: If Android is not using TCMalloc, why is this even > > trying > > > to get compiled? If it is not, why are you conditioning to preclude > > compilation > > > at this point?? Why not pull all mentions of Android? .... and if you > want > > it > > > to "sort of compile" (?) then why are you putting in a #error? > > > > FYI :) > > this was all added quite a while ago: > > https://chromiumcodereview.appspot.com/14645016 > > > > back then, myself and dmikirube needed to run deep-memory-profiler on android, > > which required TCMalloc. > > on android, we never used TCMalloc in any other scenario (production, debug, > > etc...), except when explicitly building for DMProf. > > I suppose we could try to revert that entire patch by now that there are > better > > memory profiling tools in place (and no plans for TCMalloc)? > > > > there were a few other ones too if we want to pull all android-related changes > > that were made at that time.. > > The code you linked to appears to have more than just Android porting code.... > but I would argue that IF you are removing Android support for TCMalloc, you > should clean the code more thoroughly, and remove the resulting (now) dead code. > At a minimum, any mention of "android" (in #if, etc.) should be removed. > > It is too easy to just leave dead code around.... and make the codebase > progressively less readable. This intention of this CL is not actually to remove Android support for TCMalloc compilation - the #error I added will only be triggered if ENABLE_PROFILING is defined (in the case when it is not, the EnvToXXX macros are null defined on all platforms). Thus, this will only break on Android if both the "profiling=1" and "use_allocator=tcmalloc" gyp_defines are set, but will succeed if either one or neither of them are set. I'm happy to revert all the Android porting code from TCMalloc instead if there are no plans to every use TCMalloc for any other reasons on Android though. Would you prefer this even though Android support for TCMalloc would still be possible without the use of __system_properties_get?
On 2015/01/07 22:57:59, rmcilroy wrote: > On 2015/01/07 21:53:36, jar (doing other things) wrote: > > On 2015/01/07 20:42:16, bulach wrote: > > > On 2015/01/07 20:28:21, jar (doing other things) wrote: > > > > > > > > > > https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... > > > > File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/842673002/diff/1/third_party/tcmalloc/chromiu... > > > > third_party/tcmalloc/chromium/src/base/commandlineflags.h:118: #error > > TCMalloc > > > > does not support profiling on Android > > > > On 2015/01/07 19:39:57, jar (doing other things) wrote: > > > > > I'm missing the connection to TCMalloc. > > > > > > > > > > You are removing calls to __system_property_get(), presumably because it > > may > > > > not > > > > > be available at some point. > > > > > > > > > > You have changed the code to not compile this file for Android (in fact, > > you > > > > > can't compile it on Android). > > > > > > > > > > Can you clarify the connection to TCMalloc, and explain why blocking > > > > compilation > > > > > of this file on Android is a good thing? > > > > > > > > <DOH!> I misread the file name (I stopped looking up the path after > base... > > > > src... chromium (which is about where I have the root of my tree!?!). I > now > > > see > > > > it was under third_party/tcmalloc. > > > > > > > > One obvious question: If Android is not using TCMalloc, why is this even > > > trying > > > > to get compiled? If it is not, why are you conditioning to preclude > > > compilation > > > > at this point?? Why not pull all mentions of Android? .... and if you > > want > > > it > > > > to "sort of compile" (?) then why are you putting in a #error? > > > > > > FYI :) > > > this was all added quite a while ago: > > > https://chromiumcodereview.appspot.com/14645016 > > > > > > back then, myself and dmikirube needed to run deep-memory-profiler on > android, > > > which required TCMalloc. > > > on android, we never used TCMalloc in any other scenario (production, debug, > > > etc...), except when explicitly building for DMProf. > > > I suppose we could try to revert that entire patch by now that there are > > better > > > memory profiling tools in place (and no plans for TCMalloc)? > > > > > > there were a few other ones too if we want to pull all android-related > changes > > > that were made at that time.. > > > > The code you linked to appears to have more than just Android porting code.... > > but I would argue that IF you are removing Android support for TCMalloc, you > > should clean the code more thoroughly, and remove the resulting (now) dead > code. > > At a minimum, any mention of "android" (in #if, etc.) should be removed. > > > > It is too easy to just leave dead code around.... and make the codebase > > progressively less readable. > > This intention of this CL is not actually to remove Android support for TCMalloc > compilation - the #error I added will only be triggered if ENABLE_PROFILING is > defined (in the case when it is not, the EnvToXXX macros are null defined on all > platforms). Thus, this will only break on Android if both the "profiling=1" and > "use_allocator=tcmalloc" gyp_defines are set, but will succeed if either one or > neither of them are set. > > I'm happy to revert all the Android porting code from TCMalloc instead if there > are no plans to every use TCMalloc for any other reasons on Android though. > Would you prefer this even though Android support for TCMalloc would still be > possible without the use of __system_properties_get? Can you give an example where TCMalloc is used for Android? In general, we don't land features into chrome that "might someday be used," and we'd probably nuke a feature when we decide it is not to be used. We always have revision history to look at if we "want to go back." |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
