split MEMORY_TOOL_REPLACES_ALLOCATOR into initial size and malloc.

split MEMORY_TOOL_REPLACES_ALLOCATOR into initial size and malloc.

currently all sanitizer use MEMORY_TOOL_REPLACES_ALLOCATOR defines
for setting initial size and malloc allocation.
However none memory sanitizer tools like deep-memory-profiler 
doesn't need to set initial size.
This is a patch for splitting the define up into
MEMORY_TOOL_REPLACES_ALLOCATOR, MEMORY_SANITIZER_INITIAL_SIZE.

BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177521

[JungJik, 2014-06-25 10:17:09]

PTAL :)
current DMP doesn't show all objects(like tc-webcore)
which are allocated by WTF_MAKE_FAST_ALLOCATED.

[esprehn, 2014-06-25 12:05:43]

What does ENABLE_PROFILING mean? We build and profile all the time on linux and
need PartitionAlloc to get real numbers back from pprof.

[JungJik, 2014-06-25 12:48:18]

On 2014/06/25 12:05:43, esprehn wrote:
> What does ENABLE_PROFILING mean? We build and profile all the time on linux
and
> need PartitionAlloc to get real numbers back from pprof.

oh, sorry. it's about Android.
ENABLE_PROFILING is off now as default on android.
and also TCMALLOC is not default.
both are turned on for deep-memory-profiler.
I should change the title/description.
Is there any way to use SYSTEM_MALLOC by option?

[abarth-chromium, 2014-06-25 19:26:08]

not lgtm

We profile on Android all the time and we want to use the same malloc we use in
production.

[Dai Mikurube (NOT FULLTIME), 2014-06-26 04:42:06]

Yeah, I'm aware of the problem that PartitionAlloc is not captured by dmprof. I
think about injecting gperftools (tcmalloc) hooks in PartitionAlloc, but we need
to refer third_party/tcmalloc from third_party/WebKit to do that. :(

I also don't think this change is good as abarth said. It affects many existing
profilings.

In my understanding, SYSTEM_MALLOC doesn't work with dmprof. Why do you want to
use SYSTEM_MALLOC?

[JungJik, 2014-06-26 06:59:10]

On 2014/06/26 04:42:06, Dai Mikurube wrote:
> Yeah, I'm aware of the problem that PartitionAlloc is not captured by dmprof.
I
> think about injecting gperftools (tcmalloc) hooks in PartitionAlloc, but we
need
> to refer third_party/tcmalloc from third_party/WebKit to do that. :(
> 
> I also don't think this change is good as abarth said. It affects many
existing
> profilings.
> 
> In my understanding, SYSTEM_MALLOC doesn't work with dmprof. Why do you want
to
> use SYSTEM_MALLOC?
USE_SYSTEM_MALLOC is just malloc in this context and malloc function would be
hooked by 'use_allocator="tcmalloc"' dmp build option. so finally we can get
the backtrace of WebCore objects.
--

Thanks for comments first.
I've check the ENABLE_PROFILING on Android/ChromeShell/Release.
and it was disable by default at this point in time.
So to get right backtrace of memory functions,
we shouldn't use PartitionAlloc.
because PartitionAlloc returns pre-allocated memory when objects become
instance.
if it is not good due to current profiling, could we use this optionally.

[Dai Mikurube (NOT FULLTIME), 2014-06-26 07:10:44]

I delegate final decision to abarth and/or other Blink professionals as I'm not
very familiar with PartitionAlloc and Blink-internals. I have just one comment.

From an external viewpoint, the gyp option name "use_system_malloc" is so
confusing because we already have "use_allocator". The names conflict. Also, you
intent to use tcmalloc, which is not "system malloc".

IIUC, you mean "not to use PartitionAlloc in Blink, but to use the same
allocator with Chromium", right?

If so, you should use a different name like "use_partition_alloc_in_blink
(default=1)" or "use_default_allocator_in_blink (default=0)" or something live
that.

[abarth-chromium, 2014-06-26 07:26:24]

> I've check the ENABLE_PROFILING on Android/ChromeShell/Release.
> and it was disable by default at this point in time.

Yes, but many people enable it in order to profile the product.  Switching
allocators based on this flag is too much of a change to the system and will
change CPU profiles.

> So to get right backtrace of memory functions,
> we shouldn't use PartitionAlloc.
> because PartitionAlloc returns pre-allocated memory when objects become
instance.
> if it is not good due to current profiling, could we use this optionally.

Perhaps you should search for another solution.  For example, how does ASAN with
with PartitionAlloc?

[Dai Mikurube (NOT FULLTIME), 2014-06-26 07:41:04]

jfyi, a #define flag MEMORY_TOOL_REPLACES_ALLOCATOR disables PartitionAlloc in
SOurce/wtf/PartitionAlloc.*. This flag is switched in Chromium's
build/common.gypi for sanitizers. But, this flag changes more other things.

I guess the current patch is not very problematic because it needs another
special gyp build option, not depending on ENABLE_PROFILING (while I'm -1 with
the name of the option).

[JungJik, 2014-06-26 07:58:06]

On 2014/06/26 07:41:04, Dai Mikurube wrote:
> jfyi, a #define flag MEMORY_TOOL_REPLACES_ALLOCATOR disables PartitionAlloc in
> SOurce/wtf/PartitionAlloc.*. This flag is switched in Chromium's
> build/common.gypi for sanitizers. But, this flag changes more other things.
> 
> I guess the current patch is not very problematic because it needs another
> special gyp build option, not depending on ENABLE_PROFILING (while I'm -1 with
> the name of the option).

Yes! MEMORY_TOOL_REPLACES_ALLOCATOR is what I want to use.
But it seems enabling it when we turn asan on and we can't use asan with
tcmalloc.
Could we use only MEMORY_TOOL_REPLACES_ALLOCATOR for dmp by adding gyp option
like
asan?

[Dai Mikurube (NOT FULLTIME), 2014-06-27 05:49:59]

On 2014/06/26 07:58:06, JungJik wrote:
> On 2014/06/26 07:41:04, Dai Mikurube wrote:
> > jfyi, a #define flag MEMORY_TOOL_REPLACES_ALLOCATOR disables PartitionAlloc
in
> > SOurce/wtf/PartitionAlloc.*. This flag is switched in Chromium's
> > build/common.gypi for sanitizers. But, this flag changes more other things.
> > 
> > I guess the current patch is not very problematic because it needs another
> > special gyp build option, not depending on ENABLE_PROFILING (while I'm -1
with
> > the name of the option).
> 
> Yes! MEMORY_TOOL_REPLACES_ALLOCATOR is what I want to use.
> But it seems enabling it when we turn asan on and we can't use asan with
> tcmalloc.
> Could we use only MEMORY_TOOL_REPLACES_ALLOCATOR for dmp by adding gyp option
> like
> asan?

AFAIK, there are no other gyp build options to enable
MEMORY_TOOL_REPLACES_ALLOCATOR, and it has many other side effects on memory
usage. Also the name "MEMORY_TOOL" is specific for *-sanitizers. I think you
shouldn't use this flag for your purpose.

[JungJik, 2014-06-30 02:31:47]

On 2014/06/27 05:49:59, Dai Mikurube wrote:
> On 2014/06/26 07:58:06, JungJik wrote:
> > On 2014/06/26 07:41:04, Dai Mikurube wrote:
> > > jfyi, a #define flag MEMORY_TOOL_REPLACES_ALLOCATOR disables
PartitionAlloc
> in
> > > SOurce/wtf/PartitionAlloc.*. This flag is switched in Chromium's
> > > build/common.gypi for sanitizers. But, this flag changes more other
things.
> > > 
> > > I guess the current patch is not very problematic because it needs another
> > > special gyp build option, not depending on ENABLE_PROFILING (while I'm -1
> with
> > > the name of the option).
> > 
> > Yes! MEMORY_TOOL_REPLACES_ALLOCATOR is what I want to use.
> > But it seems enabling it when we turn asan on and we can't use asan with
> > tcmalloc.
> > Could we use only MEMORY_TOOL_REPLACES_ALLOCATOR for dmp by adding gyp
option
> > like
> > asan?
> 
> AFAIK, there are no other gyp build options to enable
> MEMORY_TOOL_REPLACES_ALLOCATOR, and it has many other side effects on memory
> usage. Also the name "MEMORY_TOOL" is specific for *-sanitizers. I think you
> shouldn't use this flag for your purpose.

thanks Dai Mikurube for the comment.

if we can not use MEMORY_TOOL_REPLACES_ALLOCATOR,
can we use WTF_USE_SYSTEM_MALLOC again?

while I was testing with the flag, it was okay.
I think it's enough that if the flag is configurable by gyp.

currently we use static define which is set to 0.
so even we can not redefine it by gyp.

[abarth-chromium, 2014-06-30 02:43:11]

On 2014/06/30 at 02:31:47, jungjik.lee wrote:
> if we can not use MEMORY_TOOL_REPLACES_ALLOCATOR,
> can we use WTF_USE_SYSTEM_MALLOC again?

No, we should delete WTF_USE_SYSTEM_MALLOC from the codebase instead.  We used
to have that configurability but we no longer do.

[abarth-chromium, 2014-06-30 02:47:58]

Removing WTF_USE_SYSTEM_MALLOC in https://codereview.chromium.org/345203007

[abarth-chromium, 2014-06-30 03:18:22]

Can we split MEMORY_TOOL_REPACES_ALLOCATOR into two defines that are both
controlled from GYP?  One would control whether partion alloc routes to malloc
and the other will change all the random constants?

[JungJik, 2014-06-30 07:30:33]

On 2014/06/30 03:18:22, abarth wrote:
> Can we split MEMORY_TOOL_REPACES_ALLOCATOR into two defines that are both
> controlled from GYP?  One would control whether partion alloc routes to malloc
> and the other will change all the random constants?

thanks abarth for your comments.

do all the random constants mean constants values which is set 1 for sanitizer?
if I am right, yes we can replace MEMORY_TOOL_REPACES_ALLOCATOR to
MEMORY_SANITIZER_INITIAL_SIZE where constants has set to 1.

so all sanitizer tool use MEMORY_TOOL_REPACES_ALLOCATOR and
MEMORY_SANITIZER_INITIAL_SIZE defines.
and other can use MEMORY_TOOL_REPACES_ALLOCATOR only.

[abarth-chromium, 2014-06-30 17:31:05]

On 2014/06/30 at 07:30:33, jungjik.lee wrote:
> On 2014/06/30 03:18:22, abarth wrote:
> > Can we split MEMORY_TOOL_REPACES_ALLOCATOR into two defines that are both
> > controlled from GYP?  One would control whether partion alloc routes to
malloc
> > and the other will change all the random constants?
> 
> thanks abarth for your comments.
> 
> do all the random constants mean constants values which is set 1 for
sanitizer?

Yes.  We do that so that when code overflows, it actually causes memory errors.

> if I am right, yes we can replace MEMORY_TOOL_REPACES_ALLOCATOR to
> MEMORY_SANITIZER_INITIAL_SIZE where constants has set to 1.

Makes sense.

> so all sanitizer tool use MEMORY_TOOL_REPACES_ALLOCATOR and
MEMORY_SANITIZER_INITIAL_SIZE defines.
> and other can use MEMORY_TOOL_REPACES_ALLOCATOR only.

Yep.

[JungJik, 2014-07-03 01:52:09]

On 2014/06/30 17:31:05, abarth wrote:
> On 2014/06/30 at 07:30:33, jungjik.lee wrote:
> > On 2014/06/30 03:18:22, abarth wrote:
> > > Can we split MEMORY_TOOL_REPACES_ALLOCATOR into two defines that are both
> > > controlled from GYP?  One would control whether partion alloc routes to
> malloc
> > > and the other will change all the random constants?
> > 
> > thanks abarth for your comments.
> > 
> > do all the random constants mean constants values which is set 1 for
> sanitizer?
> 
> Yes.  We do that so that when code overflows, it actually causes memory
errors.
> 
> > if I am right, yes we can replace MEMORY_TOOL_REPACES_ALLOCATOR to
> > MEMORY_SANITIZER_INITIAL_SIZE where constants has set to 1.
> 
> Makes sense.
> 
> > so all sanitizer tool use MEMORY_TOOL_REPACES_ALLOCATOR and
> MEMORY_SANITIZER_INITIAL_SIZE defines.
> > and other can use MEMORY_TOOL_REPACES_ALLOCATOR only.
> 
> Yep.

@abarth : If you are free, PTAL again :)

[abarth-chromium, 2014-07-03 02:23:31]

lgtm

[commit-bot: I haz the power, 2014-07-03 02:24:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/357603002/60001

[commit-bot: I haz the power, 2014-07-03 03:34:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15214)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/28847)

[commit-bot: I haz the power, 2014-07-03 04:26:35]

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

[commit-bot: I haz the power, 2014-07-03 04:26:36]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/28880)

[abarth-chromium, 2014-07-03 06:33:38]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-07-03 06:34:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/357603002/60001

[commit-bot: I haz the power, 2014-07-03 08:31:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15234)

[commit-bot: I haz the power, 2014-07-03 09:48:25]

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

[commit-bot: I haz the power, 2014-07-03 09:48:25]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15253)

[JungJik, 2014-07-04 07:05:49]

The CQ bit was checked by jungjik.lee@samsung.com

[commit-bot: I haz the power, 2014-07-04 07:06:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/357603002/60001

[commit-bot: I haz the power, 2014-07-04 08:12:57]

Change committed as 177521