web-threads: Create a single instance of WebThread for each blink thread.

web-threads: Create a single instance of WebThread for each blink thread.

BlinkPlatformImpl currently creates an instance of WebThreadImpl when blink
creates a thread using 'createThread()'. When some code runs in the newly
created thread and tries to get hold of the WebThread that represents the
current thread using 'currentThread()', BlinkPlatformImpl creates a new
instance of a WebThreadImplForMessageLoop and returns that. For subsequent
calls to 'currentThread()', this same WebThreadImplForMessageLoop is
returned. So BlinkPlatformImpl ends up creating two WebThread instances
for each thread created in blink.

This patch changes this to only create a single instance of WebThread for
each thread created in blink. WebThreadImplForMessageLoop is no longer
necessary, so it is removed.

BUG=462067
Committed: https://crrev.com/e2428d07b35723722cb9579ed6074d55bd858516
Cr-Commit-Position: refs/heads/master@{#318913}

[sadrul, 2015-02-26 01:50:48]

sadrul@chromium.org changed reviewers:
+ jochen@chromium.org, kinuko@chromium.org

[sadrul, 2015-02-26 01:50:49]

Hi. Does this look like a reasonable fix? With this change,
WebThreadImplForMessageLoop is created only for the main-thread. Do you think it
would make sense to rename WebThreadImplForMessageLoop to
WebThreadImplForMainThread to make this more obvious?

[sadrul, 2015-02-26 16:36:57]

sadrul@chromium.org changed reviewers:
- jochen@chromium.org, kinuko@chromium.org

[sadrul, 2015-02-27 04:22:50]

sadrul@chromium.org changed reviewers:
+ jochen@chromium.org, kinuko@chromium.org, skyostil@chromium.org

[sadrul, 2015-02-27 04:26:00]

+skyostil@

On 2015/02/26 01:50:49, sadrul wrote:
> Hi. Does this look like a reasonable fix? With this change,
> WebThreadImplForMessageLoop is created only for the main-thread. Do you think
it
> would make sense to rename WebThreadImplForMessageLoop to
> WebThreadImplForMainThread to make this more obvious?

Turns out, after https://codereview.chromium.org/922733002,
WebThreadImplForMessageLoop isn't created for the main thread anymore. So
WebThreadImplForMessageLoop would no longer be necessary anymore. So I have
removed that completely (and reincarnated it for test-only as
WebThreadImplForTest). Please take a look.

[kinuko, 2015-02-27 09:54:27]

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
content/child/blink_platform_impl.cc:514: return
static_cast<blink::WebThread*>(current_thread_slot_.Get());
Hmm. By looking into the callsites in production code it doesn't look it's super
important to return a meaningful thread value here..

https://codereview.chromium.org/959803003/diff/180001/content/child/webthread...
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/child/webthread...
content/child/webthread_impl.cc:119: void WebThreadBase::enterRunLoop() {
Is this now used only for unit tests? If so I'd just add NOTREACHED() here and
remove MessageLoop virtual method from this interface.

[Sami, 2015-02-27 17:23:08]

Good cleanup!

https://codereview.chromium.org/959803003/diff/180001/content/child/webthread...
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/child/webthread...
content/child/webthread_impl.cc:119: void WebThreadBase::enterRunLoop() {
On 2015/02/27 09:54:27, kinuko wrote:
> Is this now used only for unit tests? If so I'd just add NOTREACHED() here and
> remove MessageLoop virtual method from this interface.

There are a handful of tests on the Blink side which call this too. Perhaps it
would be worth renaming this to enterRunLoopForTesting() or something?

https://codereview.chromium.org/959803003/diff/180001/content/test/test_blink...
File content/test/test_blink_web_unit_test_support.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/test/test_blink...
content/test/test_blink_web_unit_test_support.cc:99: web_thread_.reset(new
WebThreadImplForTest());
Since we're using the real scheduler, it seems like we should also use
WebThreadImplForScheduler here.

[sadrul, 2015-03-01 10:21:38]

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
content/child/blink_platform_impl.cc:514: return
static_cast<blink::WebThread*>(current_thread_slot_.Get());
On 2015/02/27 09:54:27, kinuko wrote:
> Hmm. By looking into the callsites in production code it doesn't look it's
super
> important to return a meaningful thread value here..

it looks like it's used in some places to add task-observers to the thread (e.g.
MemoryCache, WebDevToolsAgentImpl). Do you think those are always called on the
main-thread?

https://codereview.chromium.org/959803003/diff/180001/content/child/webthread...
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/child/webthread...
content/child/webthread_impl.cc:119: void WebThreadBase::enterRunLoop() {
On 2015/02/27 17:23:08, Sami wrote:
> On 2015/02/27 09:54:27, kinuko wrote:
> > Is this now used only for unit tests? If so I'd just add NOTREACHED() here
and
> > remove MessageLoop virtual method from this interface.
> 
> There are a handful of tests on the Blink side which call this too. Perhaps it
> would be worth renaming this to enterRunLoopForTesting() or something?

I have filed http://crbug.com/462927 to remove enterRunLoop/exitRunLoop from
WebThread interface (also some related cleanup:
https://codereview.chromium.org/962053003/). Would it make sense to leave this
as is for now and do that cleanup separately?

https://codereview.chromium.org/959803003/diff/180001/content/test/test_blink...
File content/test/test_blink_web_unit_test_support.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/test/test_blink...
content/test/test_blink_web_unit_test_support.cc:99: web_thread_.reset(new
WebThreadImplForTest());
On 2015/02/27 17:23:08, Sami wrote:
> Since we're using the real scheduler, it seems like we should also use
> WebThreadImplForScheduler here.

Sounds right. Done.

[kinuko, 2015-03-02 06:08:17]

Are the tests on try broken because of this patch?

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
content/child/blink_platform_impl.cc:514: return
static_cast<blink::WebThread*>(current_thread_slot_.Get());
On 2015/03/01 10:21:38, sadrul wrote:
> On 2015/02/27 09:54:27, kinuko wrote:
> > Hmm. By looking into the callsites in production code it doesn't look it's
> super
> > important to return a meaningful thread value here..
> 
> it looks like it's used in some places to add task-observers to the thread
(e.g.
> MemoryCache, WebDevToolsAgentImpl). Do you think those are always called on
the
> main-thread?

They belong to main thread afaik. (I'm ok to keep this one for now btw)

[sadrul, 2015-03-02 07:43:14]

On 2015/03/02 06:08:17, kinuko wrote:
> Are the tests on try broken because of this patch?

I don't think so. I have triggered new tries on the affected bots.

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
content/child/blink_platform_impl.cc:514: return
static_cast<blink::WebThread*>(current_thread_slot_.Get());
On 2015/03/02 06:08:17, kinuko wrote:
> On 2015/03/01 10:21:38, sadrul wrote:
> > On 2015/02/27 09:54:27, kinuko wrote:
> > > Hmm. By looking into the callsites in production code it doesn't look it's
> > super
> > > important to return a meaningful thread value here..
> > 
> > it looks like it's used in some places to add task-observers to the thread
> (e.g.
> > MemoryCache, WebDevToolsAgentImpl). Do you think those are always called on
> the
> > main-thread?
> 
> They belong to main thread afaik. (I'm ok to keep this one for now btw)

If this is only called on the main thread, then indeed we can git rid of the TLS
from here (in fact, it may even be possible to not have
Platform::currentThread(), but I am not sure). I would like to look into that
separately though if that makes sense.

[kinuko, 2015-03-02 07:46:03]

On 2015/03/02 07:43:14, sadrul wrote:
> On 2015/03/02 06:08:17, kinuko wrote:
> > Are the tests on try broken because of this patch?
> 
> I don't think so. I have triggered new tries on the affected bots.
> 
>
https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
> File content/child/blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
> content/child/blink_platform_impl.cc:514: return
> static_cast<blink::WebThread*>(current_thread_slot_.Get());
> On 2015/03/02 06:08:17, kinuko wrote:
> > On 2015/03/01 10:21:38, sadrul wrote:
> > > On 2015/02/27 09:54:27, kinuko wrote:
> > > > Hmm. By looking into the callsites in production code it doesn't look
it's
> > > super
> > > > important to return a meaningful thread value here..
> > > 
> > > it looks like it's used in some places to add task-observers to the thread
> > (e.g.
> > > MemoryCache, WebDevToolsAgentImpl). Do you think those are always called
on
> > the
> > > main-thread?
> > 
> > They belong to main thread afaik. (I'm ok to keep this one for now btw)
> 
> If this is only called on the main thread, then indeed we can git rid of the
TLS
> from here (in fact, it may even be possible to not have
> Platform::currentThread(), but I am not sure). I would like to look into that
> separately though if that makes sense.

As far as I looked into this, V8PerIsolateData.cpp calls this on the worker
thread, though we don't use the returned value other than checking if it's null
or non-null:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Sami, 2015-03-02 11:40:02]

lgtm, but just one question about TLS for the main thread.

Thanks for starting the enterRunLoop cleanup too!

https://codereview.chromium.org/959803003/diff/240001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/959803003/diff/240001/content/child/blink_pla...
content/child/blink_platform_impl.cc:420: current_thread_slot_() {
nit: I don't think you need to initialize current_thread_slot_ explicitly
anymore (even though the constructor is explicit).

https://codereview.chromium.org/959803003/diff/240001/content/test/test_blink...
File content/test/test_blink_web_unit_test_support.cc (right):

https://codereview.chromium.org/959803003/diff/240001/content/test/test_blink...
content/test/test_blink_web_unit_test_support.cc:324: blink::WebThread*
TestBlinkWebUnitTestSupport::currentThread() {
I'm wondering if we should just register web_thread_ into the TLS instead of
overriding this method here (and doing the same in RendererBlinkPlatformImpl)?
That would require a little less special casing, but then again TLS lookups can
be slow on some platforms. WDYT?

[kinuko, 2015-03-02 14:15:33]

lgtm 2 given that all tests are green now.

Using TLS for the main thread too sounds nice, though I'm fine with either code
if it could hurt performance.

[sadrul, 2015-03-02 16:53:21]

https://codereview.chromium.org/959803003/diff/240001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/959803003/diff/240001/content/child/blink_pla...
content/child/blink_platform_impl.cc:420: current_thread_slot_() {
On 2015/03/02 11:40:02, Sami wrote:
> nit: I don't think you need to initialize current_thread_slot_ explicitly
> anymore (even though the constructor is explicit).

Done.

https://codereview.chromium.org/959803003/diff/240001/content/test/test_blink...
File content/test/test_blink_web_unit_test_support.cc (right):

https://codereview.chromium.org/959803003/diff/240001/content/test/test_blink...
content/test/test_blink_web_unit_test_support.cc:324: blink::WebThread*
TestBlinkWebUnitTestSupport::currentThread() {
On 2015/03/02 11:40:02, Sami wrote:
> I'm wondering if we should just register web_thread_ into the TLS instead of
> overriding this method here (and doing the same in RendererBlinkPlatformImpl)?
> That would require a little less special casing, but then again TLS lookups
can
> be slow on some platforms. WDYT?

From our earlier discussion
(https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...),
it may be possible to just get rid of the TLS we maintain in BlinkPlatformImpl
since currentThread() is only called on main-thread. In that case, we would need
overrides in both RendererBlinkPlatformImpl and here.

[Sami, 2015-03-02 17:25:07]

https://codereview.chromium.org/959803003/diff/240001/content/test/test_blink...
File content/test/test_blink_web_unit_test_support.cc (right):

https://codereview.chromium.org/959803003/diff/240001/content/test/test_blink...
content/test/test_blink_web_unit_test_support.cc:324: blink::WebThread*
TestBlinkWebUnitTestSupport::currentThread() {
On 2015/03/02 16:53:21, sadrul wrote:
> On 2015/03/02 11:40:02, Sami wrote:
> > I'm wondering if we should just register web_thread_ into the TLS instead of
> > overriding this method here (and doing the same in
RendererBlinkPlatformImpl)?
> > That would require a little less special casing, but then again TLS lookups
> can
> > be slow on some platforms. WDYT?
> 
> From our earlier discussion
>
(https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...),
> it may be possible to just get rid of the TLS we maintain in BlinkPlatformImpl
> since currentThread() is only called on main-thread. In that case, we would
need
> overrides in both RendererBlinkPlatformImpl and here.

Interesting. Maybe that code should be migrated to this:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(Note: I'm in the middle of moving clients of WTF::callOnMain to that too.)

[kinuko, 2015-03-03 00:14:14]

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/959803003/diff/180001/content/child/blink_pla...
content/child/blink_platform_impl.cc:514: return
static_cast<blink::WebThread*>(current_thread_slot_.Get());
On 2015/03/02 07:43:14, sadrul wrote:
> On 2015/03/02 06:08:17, kinuko wrote:
> > On 2015/03/01 10:21:38, sadrul wrote:
> > > On 2015/02/27 09:54:27, kinuko wrote:
> > > > Hmm. By looking into the callsites in production code it doesn't look
it's
> > > super
> > > > important to return a meaningful thread value here..
> > > 
> > > it looks like it's used in some places to add task-observers to the thread
> > (e.g.
> > > MemoryCache, WebDevToolsAgentImpl). Do you think those are always called
on
> > the
> > > main-thread?
> > 
> > They belong to main thread afaik. (I'm ok to keep this one for now btw)
> 
> If this is only called on the main thread, then indeed we can git rid of the
TLS
> from here (in fact, it may even be possible to not have
> Platform::currentThread(), but I am not sure). I would like to look into that
> separately though if that makes sense.

This is not really true, it's also called on the worker thread (but only to
check if it's in unit tests or not) please see this comment:
https://codereview.chromium.org/959803003/#msg11

[jochen (gone - plz use gerrit), 2015-03-03 13:51:21]

lgtm

[sadrul, 2015-03-03 17:02:24]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-03-03 17:02:24]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/959803003/#ps260001
(title: ".")

[commit-bot: I haz the power, 2015-03-03 17:03:47]

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

[commit-bot: I haz the power, 2015-03-03 18:45:15]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-03-03 18:46:34]

Patchset 14 (id:??) landed as
https://crrev.com/e2428d07b35723722cb9579ed6074d55bd858516
Cr-Commit-Position: refs/heads/master@{#318913}