Add a background timing thread to kilobench

Add a background timing thread to kilobench

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1612513002

Committed: https://skia.googlesource.com/skia/+/b35c82dc943073e9945c0beea2d49925b45428dd

[joshua.litt, 2016-01-20 16:45:05]

Description was changed from

==========
Add a background timing thread to kilobench

BUG=skia:
==========

to

==========
Add a background timing thread to kilobench

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[joshualitt, 2016-01-20 17:29:24]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, djsollen@google.com, joshualitt@google.com,
mtklein@google.com

[joshualitt, 2016-01-20 17:29:24]

ptal

[bsalomon, 2016-01-20 18:54:52]

Is it worth putting in any instrumentation to show that the thread is actually
woken up in time to wait for the fence? If the fence is immediately completed
when we query then we're probably not measuring correctly.

https://codereview.chromium.org/1612513002/diff/80001/include/gpu/gl/SkGLCont...
File include/gpu/gl/SkGLContext.h (right):

https://codereview.chromium.org/1612513002/diff/80001/include/gpu/gl/SkGLCont...
include/gpu/gl/SkGLContext.h:68: void swapBuffers(bool usePlatformSwapBuffers =
false);
Can we just undo the swap-as-fence-sync hack before landing this?

Maybe give tools that are relying on this method an alternative
(waitOnSyncOrSwap()?) that  has the old behavior?

https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
File tools/kilobench/kilobench.cpp (right):

https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
tools/kilobench/kilobench.cpp:155: this->fGL->makeCurrent();
We don't really need all these thises (these?, this's?, those?)

https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
tools/kilobench/kilobench.cpp:317: fFenceSync->flushAndWaitFence(startSync);
Assume in light of other CL, this will not flush?

[joshualitt, 2016-01-20 21:14:52]

feedback incorporated

https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
File tools/kilobench/kilobench.cpp (right):

https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
tools/kilobench/kilobench.cpp:155: this->fGL->makeCurrent();
On 2016/01/20 18:54:52, bsalomon wrote:
> We don't really need all these thises (these?, this's?, those?)

Acknowledged.

https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
tools/kilobench/kilobench.cpp:317: fFenceSync->flushAndWaitFence(startSync);
On 2016/01/20 18:54:52, bsalomon wrote:
> Assume in light of other CL, this will not flush?

Acknowledged.

[joshualitt, 2016-01-22 19:11:52]

On 2016/01/20 21:14:52, joshualitt wrote:
> feedback incorporated
> 
>
https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
> File tools/kilobench/kilobench.cpp (right):
> 
>
https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
> tools/kilobench/kilobench.cpp:155: this->fGL->makeCurrent();
> On 2016/01/20 18:54:52, bsalomon wrote:
> > We don't really need all these thises (these?, this's?, those?)
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1612513002/diff/80001/tools/kilobench/kiloben...
> tools/kilobench/kilobench.cpp:317: fFenceSync->flushAndWaitFence(startSync);
> On 2016/01/20 18:54:52, bsalomon wrote:
> > Assume in light of other CL, this will not flush?
> 
> Acknowledged.

other feedback on this CL?

[djsollen, 2016-01-25 16:54:26]

https://codereview.chromium.org/1612513002/diff/100001/include/gpu/gl/SkGLCon...
File include/gpu/gl/SkGLContext.h (right):

https://codereview.chromium.org/1612513002/diff/100001/include/gpu/gl/SkGLCon...
include/gpu/gl/SkGLContext.h:91: SkGpuFenceSync* fenceSync() { return
fFenceSync.get(); }
does this imply that there is only ever one fence per context?

https://codereview.chromium.org/1612513002/diff/100001/tools/kilobench/kilobe...
File tools/kilobench/kilobench.cpp (right):

https://codereview.chromium.org/1612513002/diff/100001/tools/kilobench/kilobe...
tools/kilobench/kilobench.cpp:162: void endTiming(bool usePlatformSwapBuffers) {
why pass in a param that you never use?

https://codereview.chromium.org/1612513002/diff/100001/tools/kilobench/kilobe...
tools/kilobench/kilobench.cpp:168: void fence() {
seems like a poor function name when the call doesn't issue a fence but instead
a finish.

[joshualitt, 2016-01-27 17:03:03]

feedback inc

https://codereview.chromium.org/1612513002/diff/100001/include/gpu/gl/SkGLCon...
File include/gpu/gl/SkGLContext.h (right):

https://codereview.chromium.org/1612513002/diff/100001/include/gpu/gl/SkGLCon...
include/gpu/gl/SkGLContext.h:91: SkGpuFenceSync* fenceSync() { return
fFenceSync.get(); }
On 2016/01/25 16:54:25, djsollen wrote:
> does this imply that there is only ever one fence per context?

The fence sync object is actually a factory for creating fence syncs.  This just
allows owners of the SkGLContext to create their own fences

[djsollen, 2016-01-27 19:28:01]

lgtm

[joshualitt, 2016-01-27 20:53:46]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2016-01-27 20:53:49]

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

[joshualitt, 2016-01-27 20:58:53]

The CQ bit was unchecked by joshualitt@google.com

[joshualitt, 2016-01-28 14:13:59]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2016-01-28 14:13:59]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1612513002/#ps140001
(title: "kilobench logs gpu numbers")

[commit-bot: I haz the power, 2016-01-28 14:14:08]

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

[commit-bot: I haz the power, 2016-01-28 14:26:37]

Description was changed from

==========
Add a background timing thread to kilobench

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add a background timing thread to kilobench

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/b35c82dc943073e9945c0beea2d49925b45428dd
==========

[commit-bot: I haz the power, 2016-01-28 14:26:38]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/b35c82dc943073e9945c0beea2d49925b45428dd