Flapper field trial to use workerpool for sync file operations.

Flapper field trial to use workerpool for sync file operations.

Flash private file operations are synchronous, and were being
scheduled on the browser's FILE thread.  This thread can have lots of
relatively long-running stuff on it.  Add a 50% field trial to
schedule on a blocking worker-pool thread, and histogram hung-plugin
instances to see if it helps.

BUG=153383
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160026

[yzshen1, 2012-10-02 23:33:44]

https://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conte...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conte...
chrome/common/chrome_content_client.cc:302: // TODO(shess):
http://crbug.com/153383 debugging.
IIRC, at this point the field trial list hasn't been initialized, right?

That is one of the reasons why we needed to register the component-updated field
trial WinFlapper at a latter stage.

https://codereview.chromium.org/11016016/diff/2011/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_file_message_filter.cc (right):

https://codereview.chromium.org/11016016/diff/2011/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_file_message_filter.cc:107: return
BrowserThread::GetBlockingPool();
According to the comments of GetBlockingPool(), this is also used for "random
blocking operations". Is it guaranteed that it is less busy than the FILE thread
and to what extent?

Because blocking these file tasks is very bad (will block flapper && any
renderer that sends sync messages to flapper), we need to use a very responsive
task runner here.

https://codereview.chromium.org/11016016/diff/2011/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11016016/diff/2011/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2338: const char* const
kFieldTrialName = "FlapperIOThread";
static, please.

[Scott Hess - ex-Googler, 2012-10-02 23:42:31]

https://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conte...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conte...
chrome/common/chrome_content_client.cc:302: // TODO(shess):
http://crbug.com/153383 debugging.
On 2012/10/02 23:33:44, yzshen1 wrote:
> IIRC, at this point the field trial list hasn't been initialized, right?
> 
> That is one of the reasons why we needed to register the component-updated
field
> trial WinFlapper at a latter stage.

OK - would it make sense to tuck it into ConductingPepperFlashFieldTrial()?  I
was thinking of decoding how to remove all that, but as long as it's still
there, it obviously gets called at the right points.

https://codereview.chromium.org/11016016/diff/2011/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_file_message_filter.cc (right):

https://codereview.chromium.org/11016016/diff/2011/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_file_message_filter.cc:107: return
BrowserThread::GetBlockingPool();
On 2012/10/02 23:33:44, yzshen1 wrote:
> According to the comments of GetBlockingPool(), this is also used for "random
> blocking operations". Is it guaranteed that it is less busy than the FILE
thread
> and to what extent?
> 
> Because blocking these file tasks is very bad (will block flapper && any
> renderer that sends sync messages to flapper), we need to use a very
responsive
> task runner here.

I'll talk to Brett.

https://codereview.chromium.org/11016016/diff/2011/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11016016/diff/2011/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2338: const char* const
kFieldTrialName = "FlapperIOThread";
On 2012/10/02 23:33:44, yzshen1 wrote:
> static, please.

OK.  But I think it's already implicitly static, since it's a constant pointer
to constant data.

[yzshen1, 2012-10-02 23:52:10]

https://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conte...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conte...
chrome/common/chrome_content_client.cc:302: // TODO(shess):
http://crbug.com/153383 debugging.
On 2012/10/02 23:42:31, shess wrote:
> On 2012/10/02 23:33:44, yzshen1 wrote:
> > IIRC, at this point the field trial list hasn't been initialized, right?
> > 
> > That is one of the reasons why we needed to register the component-updated
> field
> > trial WinFlapper at a latter stage.
> 
> OK - would it make sense to tuck it into ConductingPepperFlashFieldTrial()?  I
> was thinking of decoding how to remove all that, but as long as it's still
> there, it obviously gets called at the right points.

I guess not, because ConductingPepperFlashFieldTrial() is called even earlier.
(It doesn't manipulate FieldTrial, it only decides whether the current platform
will be involved in a field trial.)

What do you think about putting it in the creation of PepperFileMessageFilter?

https://codereview.chromium.org/11016016/diff/2011/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_file_message_filter.cc (right):

https://codereview.chromium.org/11016016/diff/2011/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_file_message_filter.cc:107: return
BrowserThread::GetBlockingPool();
I am very curious about the result. Please let me know. :)
On 2012/10/02 23:42:31, shess wrote:
> On 2012/10/02 23:33:44, yzshen1 wrote:
> > According to the comments of GetBlockingPool(), this is also used for
"random
> > blocking operations". Is it guaranteed that it is less busy than the FILE
> thread
> > and to what extent?
> > 
> > Because blocking these file tasks is very bad (will block flapper && any
> > renderer that sends sync messages to flapper), we need to use a very
> responsive
> > task runner here.
> 
> I'll talk to Brett.

[Scott Hess - ex-Googler, 2012-10-03 19:38:51]

http://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:302: // TODO(shess):
http://crbug.com/153383 debugging.
On 2012/10/02 23:52:10, yzshen1 wrote:
> On 2012/10/02 23:42:31, shess wrote:
> > On 2012/10/02 23:33:44, yzshen1 wrote:
> > > IIRC, at this point the field trial list hasn't been initialized, right?
> > > 
> > > That is one of the reasons why we needed to register the component-updated
> > field
> > > trial WinFlapper at a latter stage.
> > 
> > OK - would it make sense to tuck it into ConductingPepperFlashFieldTrial()? 
I
> > was thinking of decoding how to remove all that, but as long as it's still
> > there, it obviously gets called at the right points.
> 
> I guess not, because ConductingPepperFlashFieldTrial() is called even earlier.
> (It doesn't manipulate FieldTrial, it only decides whether the current
platform
> will be involved in a field trial.)
> 
> What do you think about putting it in the creation of PepperFileMessageFilter?

Oops, sorry - I hadn't read this before just moving it to
IsPepperFlashEnabledByDefault(), which is the code which in the past had to have
field trials setup beforehand.

Was going to argue, but I tried your suggestion and it simplified things a lot. 
So not arguing.

Earlier when it was failing browser_tests, it was because field trials weren't
setup.  With this change I see the field trial in place when the code is
executed, so I think it's in the right order.  Same thing with the
IsPepperFlashEnabledByDefault() version, but this version is better.

[yzshen1, 2012-10-03 19:58:35]

lgtm

http://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/11016016/diff/2011/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:302: // TODO(shess):
http://crbug.com/153383 debugging.
Yay!

http://codereview.chromium.org/11016016/diff/11001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/11016016/diff/11001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_file_message_filter.cc:85: NULL));
I guess this doesn't need to be on a new line?

[Scott Hess - ex-Googler, 2012-10-03 20:04:00]

OWNERS LGTM desired from brettw.

http://codereview.chromium.org/11016016/diff/11001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/11016016/diff/11001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_file_message_filter.cc:85: NULL));
On 2012/10/03 19:58:36, yzshen1 wrote:
> I guess this doesn't need to be on a new line?

Done.

[Scott Hess - ex-Googler, 2012-10-03 20:28:51]

Sorry, am idiot, actually looping in brett for:
> OWNERS LGTM desired from brettw.

[brettw, 2012-10-03 20:40:55]

lgtm

http://codereview.chromium.org/11016016/diff/14001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/11016016/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_file_message_filter.cc:65: //
TODO(shess): http://crbug.com/153383 debugging.  Runs a field trial
It seems like this is saying "TODO run a field trial". Maybe it should say "Run
a field trial... TODO(shess) remove this when we figure out what's best.".

[commit-bot: I haz the power, 2012-10-03 22:04:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/11016016/9002

[commit-bot: I haz the power, 2012-10-04 00:35:49]

Change committed as 160026