Delay font cache building by few seconds to yield for startup resource contention.

Delay font cache building by few seconds to yield for startup resource contention.

During first renderer start there are lot of things happening simultaneously some of them are:
1. Renderer is going through all font files on the system to create a font collection.
2. Renderer loading up startup URL, accessing HTML/JS File cache, net activity etc.
3. Extension initialization.

We delay building of this cache mainly to avoid parallel font file loading along with Renderer. Some systems have significant number of font files which takes long time process.

BUG=458751, 436195
R=cpu,scottmg

Committed: https://crrev.com/787e29c61825173adaf3414ff8e3d3c0c26f529b
Cr-Commit-Position: refs/heads/master@{#320226}

Committed: https://crrev.com/476acb7860ad2bd92d1ee5d6880b1fb157ab28b1
Cr-Commit-Position: refs/heads/master@{#321029}

[scottmg, 2015-03-11 16:00:40]

Could you explain more in the description why this makes sense?

i.e. what's happening in the first 5 seconds before the cache is built, and why
it's better to delay.

[Shrikant Kelkar, 2015-03-11 19:32:59]

On 2015/03/11 16:00:40, scottmg wrote:
> Could you explain more in the description why this makes sense?
> 
> i.e. what's happening in the first 5 seconds before the cache is built, and
why
> it's better to delay.

Done. ptal.

[Peter Kasting, 2015-03-11 19:34:45]

On 2015/03/11 19:32:59, Shrikant Kelkar wrote:
> Done. ptal.

No new patch is up?

[scottmg, 2015-03-11 19:40:02]

lgtm with link to crbug.

[Peter Kasting, 2015-03-11 19:42:09]

On 2015/03/11 19:34:45, Peter Kasting wrote:
> On 2015/03/11 19:32:59, Shrikant Kelkar wrote:
> > Done. ptal.
> 
> No new patch is up?

Oh, I see, you changed the CL description.

Please consider putting significant details in code comments rather than CL
descriptions.  Future readers will be reading code, and not necessarily
expecting that context is only available if they blame-spelunk.

Should we perhaps have the renderer signal the browser when it's done messing
with fonts, instead of just guessing at the five second number?

[Shrikant Kelkar, 2015-03-11 19:44:21]

On 2015/03/11 19:42:09, Peter Kasting wrote:
> On 2015/03/11 19:34:45, Peter Kasting wrote:
> > On 2015/03/11 19:32:59, Shrikant Kelkar wrote:
> > > Done. ptal.
> > 
> > No new patch is up?
> 
> Oh, I see, you changed the CL description.
> 
> Please consider putting significant details in code comments rather than CL
> descriptions.  Future readers will be reading code, and not necessarily
> expecting that context is only available if they blame-spelunk.
> 
> Should we perhaps have the renderer signal the browser when it's done messing
> with fonts, instead of just guessing at the five second number?

Signal by renderer is a good alternative thought, but problem there is that user
might start another renderer. Every rendere is going through building collection
until we prepare cache. Once browser prepares cache renderers will start using
it.

[Peter Kasting, 2015-03-11 19:45:54]

On 2015/03/11 19:44:21, Shrikant Kelkar wrote:
> Signal by renderer is a good alternative thought, but problem there is that
user
> might start another renderer. Every rendere is going through building
collection
> until we prepare cache. Once browser prepares cache renderers will start using
> it.

So what happens in the restoring-many-tabs case?  Do we have n renderers
simultaneously enumerating all these fonts?

If so perhaps the renderers shouldn't be doing anything with fonts at all, and
we should instead be trying to get the browser to create its cache as quickly as
posisble?

[Shrikant Kelkar, 2015-03-11 19:54:55]

On 2015/03/11 19:45:54, Peter Kasting wrote:
> On 2015/03/11 19:44:21, Shrikant Kelkar wrote:
> > Signal by renderer is a good alternative thought, but problem there is that
> user
> > might start another renderer. Every rendere is going through building
> collection
> > until we prepare cache. Once browser prepares cache renderers will start
using
> > it.
> 
> So what happens in the restoring-many-tabs case?  Do we have n renderers
> simultaneously enumerating all these fonts?
> 
> If so perhaps the renderers shouldn't be doing anything with fonts at all, and
> we should instead be trying to get the browser to create its cache as quickly
as
> posisble?

Yes, in case of tab restore all renderers will go through collection if this is
the first start/update to version 39&+ from version 39-.
We thought about few ways like creating cache during installation / update time
(including some sort of wait building font cache UI.), but didn't reach to any
conclusion that time. Cache is created by Chrome utility process.

[Peter Kasting, 2015-03-11 19:56:55]

On 2015/03/11 19:54:55, Shrikant Kelkar wrote:
> Yes, in case of tab restore all renderers will go through collection if this
is
> the first start/update to version 39&+ from version 39-.
> We thought about few ways like creating cache during installation / update
time
> (including some sort of wait building font cache UI.), but didn't reach to any
> conclusion that time. Cache is created by Chrome utility process.

Only the upgrade case gets hit?  We don't incur that sort of penalty on e.g.
brand new Chrome installs?

If so, it's probably not worth trying to fix anything, especially now that we're
several versions past 39, but if the penalty for this could be widespread or
frequent, it's probably worth opening a Hotlist-Slow bug about it.

[cpu_(ooo_6.6-7.5), 2015-03-12 01:38:15]

lgtm

[Shrikant Kelkar, 2015-03-12 01:41:00]

The CQ bit was checked by shrikant@chromium.org

[Shrikant Kelkar, 2015-03-12 01:41:00]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/992413003/#ps40001 (title:
"Added more description as per comment from reviewer.")

[commit-bot: I haz the power, 2015-03-12 01:41:34]

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

[commit-bot: I haz the power, 2015-03-12 03:37:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-12 03:37:51]

Patchset 3 (id:??) landed as
https://crrev.com/787e29c61825173adaf3414ff8e3d3c0c26f529b
Cr-Commit-Position: refs/heads/master@{#320226}

[gab, 2015-03-12 12:44:42]

gab@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2015-03-12 12:44:43]

https://codereview.chromium.org/992413003/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/992413003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:266: const int kBuildFontCacheDelaySec
= 5;
I don't think 5s is enough.

From timelinev2:
https://uma.googleplex.com/timeline_v2?day_count=90&platform=W&version_tag=D&...

The median (50th percentile) is more around 8s on Beta (I don't think we can
rely on stats from M41 Stable just yet as it's not even @ 100% IIRC) and just
below 20s at the 75th percentile (which is the main percentile we're looking at
for the code purple).

Are there any benefits to starting this ASAP? If not, consider delaying it 30s+.

(also remember that "time to first paint" doesn't mean "time to tab responsive",
i.e. we don't want to flood with background tasks the second we managed to draw
a single frame)

https://codereview.chromium.org/992413003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:268: content::BrowserThread::IO,
Does this have to be on the IO thread? The IO thread's responsiveness is
extremely important, especially on startup. Looks like all
ExecuteFontCacheBuildTask is doing is launching a utility process, is the
PROCESS_LAUNCHER thread perhaps more appropriate for that?

[Vangelis Kokkevis, 2015-03-16 22:46:40]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1009403004/ by vangelis@chromium.org.

The reason for reverting is: Suspected to be causing failures on Intel Release
GPU bot.

See crbug.com/467706
.

[Shrikant Kelkar, 2015-03-17 23:32:15]

On 2015/03/16 22:46:40, Vangelis Kokkevis wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/1009403004/ by mailto:vangelis@chromium.org.
> 
> The reason for reverting is: Suspected to be causing failures on Intel Release
> GPU bot.
> 
> See crbug.com/467706
> .

Relanding after making some fixed in way utility process works
https://codereview.chromium.org/1011023002/.
I tried CL on win_chromium_rel_ng and locally using
D:\depot_tools\python276_bin\python.exe content\test\gpu\run_gpu_test.py
hardware_accelerated_feature --show-stdout --output-format=gtest
--browser=release -v --use-devtools-active-port
--extra-browser-args=--enable-logging=stderr

If it's purely timing issue then it may cause problem again and will have to
look into how to terminate utility processes during browser shutdown in general.

[Shrikant Kelkar, 2015-03-17 23:32:23]

https://codereview.chromium.org/992413003/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/992413003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:266: const int kBuildFontCacheDelaySec
= 5;
On 2015/03/12 12:44:43, gab wrote:
> I don't think 5s is enough.
> 
> From timelinev2:
>
https://uma.googleplex.com/timeline_v2?day_count=90&platform=W&version_tag=D&...
> 
> The median (50th percentile) is more around 8s on Beta (I don't think we can
> rely on stats from M41 Stable just yet as it's not even @ 100% IIRC) and just
> below 20s at the 75th percentile (which is the main percentile we're looking
at
> for the code purple).
> 
> Are there any benefits to starting this ASAP? If not, consider delaying it
30s+.
> 
> (also remember that "time to first paint" doesn't mean "time to tab
responsive",
> i.e. we don't want to flood with background tasks the second we managed to
draw
> a single frame)

Modified delay to 30s

https://codereview.chromium.org/992413003/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:268: content::BrowserThread::IO,
On 2015/03/12 12:44:43, gab wrote:
> Does this have to be on the IO thread? The IO thread's responsiveness is
> extremely important, especially on startup. Looks like all
> ExecuteFontCacheBuildTask is doing is launching a utility process, is the
> PROCESS_LAUNCHER thread perhaps more appropriate for that?

Most utility processes start on IO thread as after starting they need to send a
message to the process.

[Shrikant Kelkar, 2015-03-17 23:32:34]

The CQ bit was checked by shrikant@chromium.org

[Shrikant Kelkar, 2015-03-17 23:32:35]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/992413003/#ps60001 (title:
"Modified delay to 30 seconds.")

[commit-bot: I haz the power, 2015-03-17 23:33:15]

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

[commit-bot: I haz the power, 2015-03-18 00:25:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-18 00:26:25]

Patchset 4 (id:??) landed as
https://crrev.com/476acb7860ad2bd92d1ee5d6880b1fb157ab28b1
Cr-Commit-Position: refs/heads/master@{#321029}