Set temporary paths volatile so we don't cache them.

Set temporary paths volatile so we don't cache them.

Any path that is generated frame-to-frame should not be rendered by using the
DistanceFieldPathRenderer, because generating the initial distance field, 
uploading it and rendering it takes longer than the SoftwarePathRenderer.

BUG=skia:2935
Committed: https://skia.googlesource.com/skia/+/b3eb687f8a89eb1eacd1afb4016401eb392f66ab

[jvanverth1, 2014-10-23 17:09:50]

jvanverth@google.com changed reviewers:
+ bsalomon@google.com

[jvanverth1, 2014-10-23 17:12:21]

Not quite sure if this is the right approach (should the flag be in SkPathRef?),
but it does filter temporary paths. On Mac, desk_carsvg.skp drops from ~84ms to
~58ms. Need to test PowerVR, but it should help that case too.

[jvanverth1, 2014-10-23 17:23:04]

On 2014/10/23 17:12:21, jvanverth1 wrote:
> Not quite sure if this is the right approach (should the flag be in
SkPathRef?),
> but it does filter temporary paths. On Mac, desk_carsvg.skp drops from ~84ms
to
> ~58ms. Need to test PowerVR, but it should help that case too.

PowerVR for desk_carsvg.skp drops from 684ms (no distance field paths) to 645ms.
Old distance field path code clocks in at 771ms.

[bsalomon, 2014-10-23 17:54:09]

I think we have some work to do to avoid creating temporary paths.

Which code path is triggering the volatility in this skp?

The DF path renderer should probably be updated to apply the stroke rec and use
the stroke rec along with the original path's genID as a combined key.

https://codereview.chromium.org/677463002/diff/1/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.chromium.org/677463002/diff/1/include/core/SkPath.h#newcod...
include/core/SkPath.h:195: default. Temporary paths that are discarded after use
should be
discarded or modified?

https://codereview.chromium.org/677463002/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/677463002/diff/1/src/core/SkPath.cpp#newcode143
src/core/SkPath.cpp:143: fIsVolatile = false;
I wonder if this is correct. rewind() and reset() call this and I'm not sure
that those should affect the volatility.

https://codereview.chromium.org/677463002/diff/1/src/core/SkPath.cpp#newcode2006
src/core/SkPath.cpp:2006: fIsVolatile = false; // ?
Should we save/restore this?

https://codereview.chromium.org/677463002/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/677463002/diff/1/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:713: // of applying it here? See SkDraw.cpp
Seems like we should do this instead of making a new path here now that we get a
win from seeing the original path's gen ID.

https://codereview.chromium.org/677463002/diff/1/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:744: if (pathIsMutable) {
Maybe we should avoid this early transformation to device space since we lose
the original path's gen ID.

[jvanverth1, 2014-10-23 18:01:05]

On 2014/10/23 17:54:09, bsalomon wrote:
> I think we have some work to do to avoid creating temporary paths.
> 
> Which code path is triggering the volatility in this skp?

Two places: the PathStroker, and when we transform paths to device space for
MaskFilters.

> The DF path renderer should probably be updated to apply the stroke rec and
use
> the stroke rec along with the original path's genID as a combined key.
 
I'll add an issue for that -- right now if a path is stroked it gets passed on
to the next PathRenderer (see DistanceFieldPathRenderer::canDraw()).

[bsalomon, 2014-10-23 18:16:27]

On 2014/10/23 18:01:05, jvanverth1 wrote:
> On 2014/10/23 17:54:09, bsalomon wrote:
> > I think we have some work to do to avoid creating temporary paths.
> > 
> > Which code path is triggering the volatility in this skp?
> 
> Two places: the PathStroker, and when we transform paths to device space for
> MaskFilters.
> 
> > The DF path renderer should probably be updated to apply the stroke rec and
> use
> > the stroke rec along with the original path's genID as a combined key.
>  
> I'll add an issue for that -- right now if a path is stroked it gets passed on
> to the next PathRenderer (see DistanceFieldPathRenderer::canDraw()).

Right, and then unless it is hairline comes back around as a volatile path that
was outputted by the stroker.

Can you file a bug for the mask filter stuff too? (I don't think that should
block this, was just noting that in my previous comments).

I forgot to say earlier that I think volatile should remain on the path not the
pathref.

One, perhaps minor, optimization might be to check whether a volatile path is
already in the cache (since the current volatile path could be a copy of a
non-volatile path) and if so draw from the cache, but not *add* volatile paths
to the cache.

Anyway, my only comment that is actionable for this CL is the rewind()/reset()
thing, which I don't have a strong opinion on. And I think the comment in SkPath
should mention that paths that will be mutated, and not just shortlived, should
also be marked volatile.

[jvanverth1, 2014-10-23 20:06:19]

https://codereview.chromium.org/677463002/diff/1/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.chromium.org/677463002/diff/1/include/core/SkPath.h#newcod...
include/core/SkPath.h:195: default. Temporary paths that are discarded after use
should be
On 2014/10/23 17:54:08, bsalomon wrote:
> discarded or modified?

Done.

https://codereview.chromium.org/677463002/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/677463002/diff/1/src/core/SkPath.cpp#newcode143
src/core/SkPath.cpp:143: fIsVolatile = false;
On 2014/10/23 17:54:08, bsalomon wrote:
> I wonder if this is correct. rewind() and reset() call this and I'm not sure
> that those should affect the volatility.

Done.

https://codereview.chromium.org/677463002/diff/1/src/core/SkPath.cpp#newcode2006
src/core/SkPath.cpp:2006: fIsVolatile = false; // ?
On 2014/10/23 17:54:08, bsalomon wrote:
> Should we save/restore this?

Done.

https://codereview.chromium.org/677463002/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/677463002/diff/1/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:713: // of applying it here? See SkDraw.cpp
On 2014/10/23 17:54:09, bsalomon wrote:
> Seems like we should do this instead of making a new path here now that we get
a
> win from seeing the original path's gen ID.

Filed as Issue 3055.

https://codereview.chromium.org/677463002/diff/1/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:744: if (pathIsMutable) {
On 2014/10/23 17:54:09, bsalomon wrote:
> Maybe we should avoid this early transformation to device space since we lose
> the original path's gen ID.

Filed as Issue 3056

[jvanverth1, 2014-10-23 20:09:59]

Might want to wait to review -- I need to add the mutable/isVolatile assert in
drawPath() and check for other temp paths.

[jvanverth1, 2014-10-23 20:31:12]

On 2014/10/23 20:09:59, jvanverth1 wrote:
> Might want to wait to review -- I need to add the mutable/isVolatile assert in
> drawPath() and check for other temp paths.

Ok, should be good to go. PTAL.

[bsalomon, 2014-10-23 20:36:28]

https://codereview.chromium.org/677463002/diff/60001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/677463002/diff/60001/src/core/SkPath.cpp#newc...
src/core/SkPath.cpp:134: this->resetFields();
Don't you need to set volatile=false here now?

[bsalomon, 2014-10-23 21:12:52]

On 2014/10/23 20:36:28, bsalomon wrote:
> https://codereview.chromium.org/677463002/diff/60001/src/core/SkPath.cpp
> File src/core/SkPath.cpp (right):
> 
>
https://codereview.chromium.org/677463002/diff/60001/src/core/SkPath.cpp#newc...
> src/core/SkPath.cpp:134: this->resetFields();
> Don't you need to set volatile=false here now?

lgtm, otherwise.

[jvanverth1, 2014-10-24 01:13:08]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-10-24 01:14:29]

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

[commit-bot: I haz the power, 2014-10-24 01:18:05]

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

[commit-bot: I haz the power, 2014-10-24 01:18:05]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[jvanverth1, 2014-10-24 13:38:16]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-10-24 13:39:02]

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

[commit-bot: I haz the power, 2014-10-24 13:46:25]

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

[commit-bot: I haz the power, 2014-10-24 13:46:26]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[jvanverth1, 2014-10-24 13:51:55]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-10-24 13:52:02]

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

[commit-bot: I haz the power, 2014-10-24 14:12:55]

Committed patchset #8 (id:140001) as b3eb687f8a89eb1eacd1afb4016401eb392f66ab