UserGestureIndicator: remove many unnecessary calls to isMainThread

UserGestureIndicator: remove many unnecessary calls to isMainThread

Most code that calls the UserGestureIndicator knows which thread it is
executing in. Calling isMainThread involves doing a thread id syscall which
can be quite slow, and there are places in the loading code that do this
in a tight loop (e.g. preloading resources).

This patch adds an optional argument to the indicator's static methods which
optionally allow callers to get the gesture state in a thread-safe way.

BUG=348655
Committed: https://crrev.com/8bcf27f192944e2eea90a846156a770d4ca939fc
Cr-Commit-Position: refs/heads/master@{#437941}

[Charlie Harrison, 2016-12-07 21:59:31]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:00:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-07 22:11:23]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:11:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 23:41:40]

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

[commit-bot: I haz the power, 2016-12-07 23:41:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Charlie Harrison, 2016-12-07 23:59:20]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 23:59:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-08 00:02:19]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 00:02:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 00:37:00]

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

[commit-bot: I haz the power, 2016-12-08 00:37:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Harrison, 2016-12-08 01:24:28]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 01:25:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 02:13:44]

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

[commit-bot: I haz the power, 2016-12-08 02:13:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Charlie Harrison, 2016-12-08 02:44:10]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 02:44:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-08 02:51:04]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 02:51:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 04:08:36]

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

[commit-bot: I haz the power, 2016-12-08 04:08:37]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-12-08 05:17:25]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 05:17:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 06:42:39]

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

[commit-bot: I haz the power, 2016-12-08 06:42:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Charlie Harrison, 2016-12-08 14:01:26]

Description was changed from

==========
[test] remove isMainThread() check in currentToken()

BUG=
==========

to

==========
UserGestureIndicator: remove many unnecessary calls to isMainThread

Most code that calls the UserGestureIndicator knows which thread it is
executing in. Calling isMainThread involves doing a thread id syscall which
can be quite slow, and there are places in the loading code that do this
in a tight loop (e.g. preloading resources).

This patch adds an optional argument to the indicator's static methods which
optionally allow callers to get the gesture state in a thread-safe way.

BUG=348655
==========

[Charlie Harrison, 2016-12-08 14:04:26]

csharrison@chromium.org changed reviewers:
+ kinuko@chromium.org

[Charlie Harrison, 2016-12-08 14:04:27]

kinuko: WDYT about this patch?  I mostly relied on test coverage to find places
where we need to do the thread-safe call.

[Charlie Harrison, 2016-12-08 14:06:34]

I'll add that this patch is a draft and I'm looking for a high level review of
if this is the best way to achieve my goal (fewer syscalls). Polish/comments can
be added later.

[kinuko, 2016-12-09 03:06:56]

Is getting thread id still slow?  I think it's related to the same issue as
crbug.com/497226 right?

[kinuko, 2016-12-09 03:15:52]

If it's making the code actually slow I think this change makes sense.  In blink
we often specialize thread-safe code to optimize single-threaded code path.

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:106: static bool
processingUserGesture(ThreadCheck = DoNotCheckThreadState);
I slightly prefer having processingUserGestureThreadSafe here too over taking
additional argument.

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:112: static bool
utilizeUserGesture();
Should this also check ThreadCheck?

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebUserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebUserGestureIndicator.h:46: // Can be called
from any thread.
Also note that it's slow and the other one must be preferred unless really
needed

[Charlie Harrison, 2016-12-09 03:55:44]

On 2016/12/09 03:06:56, kinuko wrote:
> Is getting thread id still slow?  I think it's related to the same issue as
> crbug.com/497226 right?

Yes this showed up as a fairly significant time in perf traces on Linux, on
release / official build. Not sure if things are different on different OSes. In
any case, unless we cache the id we'll still have syscall overhead which isn't
great.

[Charlie Harrison, 2016-12-09 15:09:33]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 15:09:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-09 15:24:14]

Thanks for the review!

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:106: static bool
processingUserGesture(ThreadCheck = DoNotCheckThreadState);
On 2016/12/09 03:15:51, kinuko wrote:
> I slightly prefer having processingUserGestureThreadSafe here too over taking
> additional argument.

Do you feel that way for all the methods? i.e. should I just scrap the enum?

I've done so in the current patch.

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:112: static bool
utilizeUserGesture();
On 2016/12/09 03:15:51, kinuko wrote:
> Should this also check ThreadCheck?

If we go with the enum approach, sure. Otherwise I don't think any existing
callers need the thread safe version so I don't think we need
utilizeUserGestureThreadSafe.

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebUserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebUserGestureIndicator.h:46: // Can be called
from any thread.
On 2016/12/09 03:15:51, kinuko wrote:
> Also note that it's slow and the other one must be preferred unless really
> needed

Done.

[kinuko, 2016-12-12 04:30:20]

Btw is the callsite in the RenderFrameImpl::willSendRequest() the one you're
trying to optimize?

[kinuko, 2016-12-12 04:45:13]

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:118: static bool
consumeUserGestureThreadSafe();
We don't need this and currentTokenThreadSafe other than in
processingUserGestureThreadSafe?

If so I started to think we should probably just keep the non-thread-safe
versions but add a thread-safety check at the callsites for these two.  (Sorry
for saying opposite to my previous comments)

[Charlie Harrison, 2016-12-12 13:10:09]

Yeah the call in willSendRequest is my biggest worry. I haven't audited all
callsites but I imagine there are a few that could potentially be in the
critical path.

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:118: static bool
consumeUserGestureThreadSafe();
On 2016/12/12 04:45:12, kinuko wrote:
> We don't need this and currentTokenThreadSafe other than in
> processingUserGestureThreadSafe?
> 
> If so I started to think we should probably just keep the non-thread-safe
> versions but add a thread-safety check at the callsites for these two.  (Sorry
> for saying opposite to my previous comments)

We use currentTokenThreadSafe in DOMTimer, and we use this and
currentTokenThreadSafe in the WebUserGestureIndicator. These are few enough
locations that we can make the isMainThread() call at the callsites if you
prefer though.

[kinuko, 2016-12-12 14:51:26]

lgtm

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:98: // non-suffixed
counterparts *must* be called on the main thread.
Let's add: "Consider always using the non-suffixed one unless the code really
needs to be thread-safe" or something like that

[Charlie Harrison, 2016-12-12 15:20:47]

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:98: // non-suffixed
counterparts *must* be called on the main thread.
On 2016/12/12 14:51:26, kinuko wrote:
> Let's add: "Consider always using the non-suffixed one unless the code really
> needs to be thread-safe" or something like that

Done.

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:118: static bool
consumeUserGestureThreadSafe();
On 2016/12/12 13:10:09, Charlie Harrison wrote:
> On 2016/12/12 04:45:12, kinuko wrote:
> > We don't need this and currentTokenThreadSafe other than in
> > processingUserGestureThreadSafe?
> > 
> > If so I started to think we should probably just keep the non-thread-safe
> > versions but add a thread-safety check at the callsites for these two. 
(Sorry
> > for saying opposite to my previous comments)
> 
> We use currentTokenThreadSafe in DOMTimer, and we use this and
> currentTokenThreadSafe in the WebUserGestureIndicator. These are few enough
> locations that we can make the isMainThread() call at the callsites if you
> prefer though.

I don't think this comment was properly resolved. Is your L-G contingent on
removing these ThreadSafes and moving the isMainThread() check to callsites, or
are these enough places where having the ThreadSafe API makes sense?

[kinuko, 2016-12-12 15:37:49]

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/2555013004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/UserGestureIndicator.h:118: static bool
consumeUserGestureThreadSafe();
On 2016/12/12 15:20:47, Charlie Harrison wrote:
> On 2016/12/12 13:10:09, Charlie Harrison wrote:
> > On 2016/12/12 04:45:12, kinuko wrote:
> > > We don't need this and currentTokenThreadSafe other than in
> > > processingUserGestureThreadSafe?
> > > 
> > > If so I started to think we should probably just keep the non-thread-safe
> > > versions but add a thread-safety check at the callsites for these two. 
> (Sorry
> > > for saying opposite to my previous comments)
> > 
> > We use currentTokenThreadSafe in DOMTimer, and we use this and
> > currentTokenThreadSafe in the WebUserGestureIndicator. These are few enough
> > locations that we can make the isMainThread() call at the callsites if you
> > prefer though.
> 
> I don't think this comment was properly resolved. Is your L-G contingent on
> removing these ThreadSafes and moving the isMainThread() check to callsites,
or
> are these enough places where having the ThreadSafe API makes sense?

Sorry I hadn't explicitly responded to this one. I had forgotten the use case in
DOMTimer so I thought it probably gives a reason to have the thread-safe version
for this one-- but then I looked the code site again and now I have another
comment :)

https://codereview.chromium.org/2555013004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):

https://codereview.chromium.org/2555013004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMTimer.cpp:94: m_userGestureToken =
UserGestureIndicator::currentTokenThreadSafe();
Looks like we only take this code path if shouldForwardUserGesture returns true,
that never happens if we're on non-main-thread because shouldForwardUserGesture
calls processingUserGestureThreadSafe().  Is my reading right?

[Charlie Harrison, 2016-12-12 15:41:30]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 15:41:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-12 15:44:38]

csharrison@chromium.org changed reviewers:
+ haraken@chromium.org, reillyg@chromium.org

[Charlie Harrison, 2016-12-12 15:44:39]

Thanks!
+haraken for web and modules
+reillyg for extensions

https://codereview.chromium.org/2555013004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right):

https://codereview.chromium.org/2555013004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/DOMTimer.cpp:94: m_userGestureToken =
UserGestureIndicator::currentTokenThreadSafe();
On 2016/12/12 15:37:49, kinuko wrote:
> Looks like we only take this code path if shouldForwardUserGesture returns
true,
> that never happens if we're on non-main-thread because
shouldForwardUserGesture
> calls processingUserGestureThreadSafe().  Is my reading right?

Yes you're right. Let's move this one to assume we're on the main thread. I also
added a brief comment.

[haraken, 2016-12-12 15:48:30]

LGTM

> Calling isMainThread involves doing a thread id syscall which can be quite
slow

ThreadState::current()->isMainThread should be much faster. Maybe we should
replace the implementation of isMainThread with the ThreadState version (though
it will need some refactoring since ThreadState is in platform/ whereas we
sometimes want to use isMainThread in wtf/)...

[Charlie Harrison, 2016-12-12 15:50:51]

On 2016/12/12 15:48:30, haraken wrote:
> LGTM
> 
> > Calling isMainThread involves doing a thread id syscall which can be quite
> slow
> 
> ThreadState::current()->isMainThread should be much faster. Maybe we should
> replace the implementation of isMainThread with the ThreadState version
(though
> it will need some refactoring since ThreadState is in platform/ whereas we
> sometimes want to use isMainThread in wtf/)...

Thank you for the suggestion. Would this sort of change be more possible when
wtf is merged into platform?

[haraken, 2016-12-12 15:57:52]

On 2016/12/12 15:50:51, Charlie Harrison wrote:
> On 2016/12/12 15:48:30, haraken wrote:
> > LGTM
> > 
> > > Calling isMainThread involves doing a thread id syscall which can be quite
> > slow
> > 
> > ThreadState::current()->isMainThread should be much faster. Maybe we should
> > replace the implementation of isMainThread with the ThreadState version
> (though
> > it will need some refactoring since ThreadState is in platform/ whereas we
> > sometimes want to use isMainThread in wtf/)...
> 
> Thank you for the suggestion. Would this sort of change be more possible when
> wtf is merged into platform?

Yes, it should become easier :)

[commit-bot: I haz the power, 2016-12-12 17:27:13]

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

[commit-bot: I haz the power, 2016-12-12 17:27:14]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-12-12 21:29:12]

lgtm

[Charlie Harrison, 2016-12-12 21:32:18]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-12-12 21:32:18]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2555013004/#ps220001
(title: "Avoid duplicate thread checks in DOMTimer")

[commit-bot: I haz the power, 2016-12-12 21:35:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 22:46:48]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1481578338445840,
"parent_rev": "e3e487e6b821ca1566b27d94fcff8d8c4b0f8d51", "commit_rev":
"ac4bf7a33cb97cf798c6b414095e8985254629ca"}

[commit-bot: I haz the power, 2016-12-12 22:47:18]

Description was changed from

==========
UserGestureIndicator: remove many unnecessary calls to isMainThread

Most code that calls the UserGestureIndicator knows which thread it is
executing in. Calling isMainThread involves doing a thread id syscall which
can be quite slow, and there are places in the loading code that do this
in a tight loop (e.g. preloading resources).

This patch adds an optional argument to the indicator's static methods which
optionally allow callers to get the gesture state in a thread-safe way.

BUG=348655
==========

to

==========
UserGestureIndicator: remove many unnecessary calls to isMainThread

Most code that calls the UserGestureIndicator knows which thread it is
executing in. Calling isMainThread involves doing a thread id syscall which
can be quite slow, and there are places in the loading code that do this
in a tight loop (e.g. preloading resources).

This patch adds an optional argument to the indicator's static methods which
optionally allow callers to get the gesture state in a thread-safe way.

BUG=348655

Review-Url: https://codereview.chromium.org/2555013004
==========

[commit-bot: I haz the power, 2016-12-12 22:47:20]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-12-12 22:51:18]

Description was changed from

==========
UserGestureIndicator: remove many unnecessary calls to isMainThread

Most code that calls the UserGestureIndicator knows which thread it is
executing in. Calling isMainThread involves doing a thread id syscall which
can be quite slow, and there are places in the loading code that do this
in a tight loop (e.g. preloading resources).

This patch adds an optional argument to the indicator's static methods which
optionally allow callers to get the gesture state in a thread-safe way.

BUG=348655

Review-Url: https://codereview.chromium.org/2555013004
==========

to

==========
UserGestureIndicator: remove many unnecessary calls to isMainThread

Most code that calls the UserGestureIndicator knows which thread it is
executing in. Calling isMainThread involves doing a thread id syscall which
can be quite slow, and there are places in the loading code that do this
in a tight loop (e.g. preloading resources).

This patch adds an optional argument to the indicator's static methods which
optionally allow callers to get the gesture state in a thread-safe way.

BUG=348655

Committed: https://crrev.com/8bcf27f192944e2eea90a846156a770d4ca939fc
Cr-Commit-Position: refs/heads/master@{#437941}
==========

[commit-bot: I haz the power, 2016-12-12 22:51:19]

Patchset 12 (id:??) landed as
https://crrev.com/8bcf27f192944e2eea90a846156a770d4ca939fc
Cr-Commit-Position: refs/heads/master@{#437941}