skia expects to be initialized before it can be used. This must be done if serializing SkPicture, w…

skia expects to be initialized before it can be used. This must be done if serializing SkPicture, which we now do for benchmarking.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150609

[alokp, 2012-08-01 23:28:06]

reed@ can provide more details on why skia needs to be initialized.

[jamesr, 2012-08-01 23:29:26]

On 2012/08/01 23:28:06, Alok Priyadarshi wrote:
> reed@ can provide more details on why skia needs to be initialized.

I'm interested in those details - could you please say what exactly is being
initialized / destroyed here and why?

[reed1, 2012-08-02 12:05:04]

Init() performs a registration step for known effect subclasses. This is
required so that during picture serialization/deserialization, we can basically
perform reflection on those virtual classes and know how to reconstruct them.

This used to be done during library-load, using global constructors, but those
have recently been removed.

[alokp, 2012-08-06 16:47:14]

Picture serialization is currently only done for benchmarking purposes. If this
is only needed for picture serialization, a more localized solution is possible.

James: let me know if would like to see the localized solution as well.

[jamesr, 2012-08-07 03:49:38]

I think if this is only used for an off-by-default path I'd prefer initializing
only when needed.  The ban on static initializers is meant to avoid the hit on
process startup time of running lots of code.  This is doing effectively the
same thing by running this code unconditionally in the process' main().

[alokp, 2012-08-07 16:56:23]

A new patch that calls SkGraphics::Init() on an as needed basis.

[jamesr, 2012-08-07 20:51:26]

http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
content/renderer/gpu/gpu_benchmarking_extension.cc:41: SkGraphics::Term();
this doesn't appear to actually be related to SkGraphics::Init() at all, based
on the comment.  What does it do?  Do you need to do anything at process
termination?  Keep in mind that in many cases we won't call this at all when
terminating a renderer, so you can't rely on anything being called in here for
correctness.  If you don't need it, I would strongly prefer not bothering with
it - AtExitManager stuff is almost always more trouble than it's worth.

The current implementation just calls PurgeFontCache().  I can't think of any
reason why we would want to purge the font cache if we're about to exit the
process anyway.

http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
content/renderer/gpu/gpu_benchmarking_extension.cc:46: if (!init) {
do we only ever call this from one thread?

[reed1, 2012-08-07 21:14:52]

http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
content/renderer/gpu/gpu_benchmarking_extension.cc:41: SkGraphics::Term();
On 2012/08/07 20:51:26, jamesr wrote:
> this doesn't appear to actually be related to SkGraphics::Init() at all, based
> on the comment.  What does it do?  Do you need to do anything at process
> termination?  Keep in mind that in many cases we won't call this at all when
> terminating a renderer, so you can't rely on anything being called in here for
> correctness.  If you don't need it, I would strongly prefer not bothering with
> it - AtExitManager stuff is almost always more trouble than it's worth.
> 
> The current implementation just calls PurgeFontCache().  I can't think of any
> reason why we would want to purge the font cache if we're about to exit the
> process anyway.

Term() is useful if you're tracking memory leaks, but it is never required for
correctness.

[jamesr, 2012-08-07 21:16:10]

On 2012/08/07 21:14:52, reed1 wrote:
>
http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
> File content/renderer/gpu/gpu_benchmarking_extension.cc (right):
> 
>
http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
> content/renderer/gpu/gpu_benchmarking_extension.cc:41: SkGraphics::Term();
> On 2012/08/07 20:51:26, jamesr wrote:
> > this doesn't appear to actually be related to SkGraphics::Init() at all,
based
> > on the comment.  What does it do?  Do you need to do anything at process
> > termination?  Keep in mind that in many cases we won't call this at all when
> > terminating a renderer, so you can't rely on anything being called in here
for
> > correctness.  If you don't need it, I would strongly prefer not bothering
with
> > it - AtExitManager stuff is almost always more trouble than it's worth.
> > 
> > The current implementation just calls PurgeFontCache().  I can't think of
any
> > reason why we would want to purge the font cache if we're about to exit the
> > process anyway.
> 
> Term() is useful if you're tracking memory leaks, but it is never required for
> correctness.

OK.  It doesn't seem related to SkGraphics::Init() - is that correct?  If so
then the call should go somewhere else (if we have it at all)

[reed1, 2012-08-07 21:18:56]

On 2012/08/07 21:16:10, jamesr wrote:
> On 2012/08/07 21:14:52, reed1 wrote:
> >
>
http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
> > File content/renderer/gpu/gpu_benchmarking_extension.cc (right):
> > 
> >
>
http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
> > content/renderer/gpu/gpu_benchmarking_extension.cc:41: SkGraphics::Term();
> > On 2012/08/07 20:51:26, jamesr wrote:
> > > this doesn't appear to actually be related to SkGraphics::Init() at all,
> based
> > > on the comment.  What does it do?  Do you need to do anything at process
> > > termination?  Keep in mind that in many cases we won't call this at all
when
> > > terminating a renderer, so you can't rely on anything being called in here
> for
> > > correctness.  If you don't need it, I would strongly prefer not bothering
> with
> > > it - AtExitManager stuff is almost always more trouble than it's worth.
> > > 
> > > The current implementation just calls PurgeFontCache().  I can't think of
> any
> > > reason why we would want to purge the font cache if we're about to exit
the
> > > process anyway.
> > 
> > Term() is useful if you're tracking memory leaks, but it is never required
for
> > correctness.
> 
> OK.  It doesn't seem related to SkGraphics::Init() - is that correct?  If so
> then the call should go somewhere else (if we have it at all)

I think you can just drop it. If later we want to track memory, or we add some
other value in Term(), we can decide where to add it then.

[alokp, 2012-08-08 17:24:34]

Removed AtExitManager usage.

http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

http://codereview.chromium.org/10823129/diff/5002/content/renderer/gpu/gpu_be...
content/renderer/gpu/gpu_benchmarking_extension.cc:46: if (!init) {
On 2012/08/07 20:51:26, jamesr wrote:
> do we only ever call this from one thread?

Yes - added comment.

[jamesr, 2012-08-08 17:42:43]

lgtm

[commit-bot: I haz the power, 2012-08-08 18:12:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/10823129/12001

[commit-bot: I haz the power, 2012-08-08 19:41:54]

Try job failure for 10823129-12001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...