Oilpan: Centralize Partition allocators into Partitions.h (part 2)

Oilpan: Centralize Partition allocators into Partitions.h (part 2)

This CL moves FastMalloc's allocator (i.e., FastMalloc::gPartition)
into Partitions.h.

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

[haraken, 2015-03-31 11:40:21]

haraken@chromium.org changed reviewers:
+ cevans@chromium.org, tsepez@chromium.org

[haraken, 2015-03-31 11:40:22]

PTAL

[Tom Sepez, 2015-03-31 16:36:16]

Why is it safe to remove the lock around initialization?

[haraken, 2015-03-31 23:11:30]

On 2015/03/31 16:36:16, Tom Sepez wrote:
> Why is it safe to remove the lock around initialization?

The lock is held in Partitions::initialize(). Wouldn't it be enough?

[Tom Sepez, 2015-04-01 01:02:25]

On 2015/03/31 23:11:30, haraken wrote:
> On 2015/03/31 16:36:16, Tom Sepez wrote:
> > Why is it safe to remove the lock around initialization?
> 
> The lock is held in Partitions::initialize(). Wouldn't it be enough?


Uh, yeah. Sorry I didn't notice that the first time around. LGTM.

[haraken, 2015-04-01 01:06:09]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-04-01 01:06:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1044143002/1

[commit-bot: I haz the power, 2015-04-01 01:17:06]

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

[commit-bot: I haz the power, 2015-04-01 01:17:07]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[haraken, 2015-04-01 01:18:04]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org

[haraken, 2015-04-01 01:18:04]

tkent-san: Would you take a look as an OWNER?

[tkent, 2015-04-01 01:26:09]

https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h
File Source/wtf/Partitions.h (right):

https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h#new...
Source/wtf/Partitions.h:53: initialize();
Why is this necessary?  Is it possible to call getFastMallocPartition() before
WTF::initialize()?

[haraken, 2015-04-01 01:33:07]

https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h
File Source/wtf/Partitions.h (right):

https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h#new...
Source/wtf/Partitions.h:53: initialize();
On 2015/04/01 01:26:09, tkent wrote:
> Why is this necessary?  Is it possible to call getFastMallocPartition() before
> WTF::initialize()?

Yes, it's possible. The fastmalloc allocator (and the buffer allocator (for
Strings etc)) can be called by DEFINE_STATIC_LOCAL etc.

[tkent, 2015-04-01 01:35:37]

lgtm

https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h
File Source/wtf/Partitions.h (right):

https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h#new...
Source/wtf/Partitions.h:53: initialize();
On 2015/04/01 01:33:07, haraken wrote:
> On 2015/04/01 01:26:09, tkent wrote:
> > Why is this necessary?  Is it possible to call getFastMallocPartition()
before
> > WTF::initialize()?
> 
> Yes, it's possible. The fastmalloc allocator (and the buffer allocator (for
> Strings etc)) can be called by DEFINE_STATIC_LOCAL etc.

I see.  Thanks.

Adding comments to getFastMallocPartition and getBufferPartition would be
helpful.

[haraken, 2015-04-01 01:35:46]

On 2015/04/01 01:33:07, haraken wrote:
> https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h
> File Source/wtf/Partitions.h (right):
> 
>
https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h#new...
> Source/wtf/Partitions.h:53: initialize();
> On 2015/04/01 01:26:09, tkent wrote:
> > Why is this necessary?  Is it possible to call getFastMallocPartition()
before
> > WTF::initialize()?
> 
> Yes, it's possible. The fastmalloc allocator (and the buffer allocator (for
> Strings etc)) can be called by DEFINE_STATIC_LOCAL etc.

Hmm, this explanation might not be correct, since DEFINE_STATIC_LOCAL shouldn't
be called until the function that contains the DEFINE_STATIC_LOCAL is called.

Let me check who is using the allocator before WTF::initialize(). (Intuitively,
there will be someone who uses it because we cannot do anything without
allocating memory in Blink.)

[Chris Evans, 2015-04-01 01:38:48]

https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h
File Source/wtf/Partitions.h (right):

https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h#new...
Source/wtf/Partitions.h:53: initialize();
On 2015/04/01 01:33:07, haraken wrote:
> On 2015/04/01 01:26:09, tkent wrote:
> > Why is this necessary?  Is it possible to call getFastMallocPartition()
before
> > WTF::initialize()?
> 
> Yes, it's possible. The fastmalloc allocator (and the buffer allocator (for
> Strings etc)) can be called by DEFINE_STATIC_LOCAL etc.

Is that fixable I wonder? It'd be nice to remove that if statement from the hot
paths :) Or maybe it's not measurable.

[haraken, 2015-04-01 01:53:50]

On 2015/04/01 01:38:48, Chris Evans wrote:
> https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h
> File Source/wtf/Partitions.h (right):
> 
>
https://codereview.chromium.org/1044143002/diff/1/Source/wtf/Partitions.h#new...
> Source/wtf/Partitions.h:53: initialize();
> On 2015/04/01 01:33:07, haraken wrote:
> > On 2015/04/01 01:26:09, tkent wrote:
> > > Why is this necessary?  Is it possible to call getFastMallocPartition()
> before
> > > WTF::initialize()?
> > 
> > Yes, it's possible. The fastmalloc allocator (and the buffer allocator (for
> > Strings etc)) can be called by DEFINE_STATIC_LOCAL etc.
> 
> Is that fixable I wonder? It'd be nice to remove that if statement from the
hot
> paths :) Or maybe it's not measurable.

I guess it's fixable, since the places we use the allocator before
WTF::initialize look limited. If I insert RELEASE_ASSERT(s_initialized),
unittests and content_shell just work fine whereas chrome crashes in the
following path. I think we should fix the call sites so that it doesn't call in
Blink before calling WTF::initialize.

WTF::fastMalloc (n=<optimized out>) at
../../third_party/WebKit/Source/wtf/FastMalloc.cpp:56
56          return partitionAllocGeneric(Partitions::getFastMallocPartition(),
n);
(gdb) bt
#0  WTF::fastMalloc (n=<optimized out>) at
../../third_party/WebKit/Source/wtf/FastMalloc.cpp:56
#1  0x0000555557d7398c in malloc<void*, WTF::Vector<v8::Extension*, 0,
WTF::DefaultAllocator> > (size=16)
    at ../../third_party/WebKit/Source/wtf/DefaultAllocator.h:111
#2  operator new (size=16) at ../../third_party/WebKit/Source/wtf/Vector.h:595
#3  registeredExtensions () at
../../third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:326
#4  blink::ScriptController::registerExtensionIfNeeded (extension=0x7e4f7de1230)
    at ../../third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:332
#5  0x0000555559405b58 in extensions::Dispatcher::Dispatcher
(this=0x7e4f7e4fa00, delegate=<optimized out>)
    at ../../extensions/renderer/dispatcher.cc:203
#6  0x0000555556451b7f in ChromeContentRendererClient::RenderThreadStarted (
    this=0x55555bed7e80 <g_chrome_content_renderer_client+8>)
    at ../../chrome/renderer/chrome_content_renderer_client.cc:374
#7  0x00005555589eae77 in content::RenderThreadImpl::Init (this=0x7e4f7e28880)
    at ../../content/renderer/render_thread_impl.cc:558
#8  0x00005555589eb8d8 in content::RenderThreadImpl::RenderThreadImpl
(this=0x7e4f7e28880, params=...)
    at ../../content/renderer/render_thread_impl.cc:452
#9  0x0000555556578346 in base::Thread::ThreadMain (this=0x7e4f7dd4a20) at
../../base/threading/thread.cc:232
#10 0x0000555556574c5c in base::(anonymous namespace)::ThreadFunc
(params=<optimized out>)
    at ../../base/threading/platform_thread_posix.cc:77
#11 0x00007ffff2c0c182 in start_thread (arg=0x7fffc9221700) at
pthread_create.c:312
#12 0x00007ffff16cb47d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111

At the moment, I'll add a TODO and land this CL with the initialization check.

[haraken, 2015-04-01 01:55:51]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-04-01 01:55:52]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1044143002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-04-01 01:56:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1044143002/20001

[tkent, 2015-04-01 01:56:27]

> At the moment, I'll add a TODO and land this CL with the initialization check.

It makes sense.  Please go ahead.

[commit-bot: I haz the power, 2015-04-01 03:23:03]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192899