New issue
Advanced search Search tips

Issue 704939 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

20% regression on Event.Latency.TouchToFirstScrollUpdateSwapBegin for Beta 57

Project Member Reported by nzolghadr@chromium.org, Mar 24 2017

Issue description

The regression happened on Feb 10 2017 version Beta 57.0.2987.38.

https://uma.googleplex.com/timeline_v2?sid=169fc7d0926e05936a156e30ab0a3330

It seems that it happened on Beta on Feb 10th, 2017.
 
Description: Show this description
Intersecting the Dev 58.0.3000.6..58.0.3007.3 range with Beta 57.0.2987.19..57.0.2987.38 shows the following commits:

be1c7be7700037d82e0e92f66232795832f72101 Don't cache WindowAndroid and ChromeActivity
68c2b21598d1ffb959056909f6b4fa8bf7a17155 Pass notification deadline timestamp as a jlong
b0a29222e2a01324bce86c9a1880678cedcb4db1 [wrapper-tracing] Reset TLS on disposing
4a8cf9449a9df503e1993433249a9e6923b5d810 Expose thumbnails of pages to iOS share extensions.
5f067c6fa15e295d45c51946470567d5cd1baca3 Remove the iOS QR Code scanner experiment code.
524a361c86c23bb4f5384c1738f786532a296e46 Fix using theme colors for the download shelf's close button.
3c36d59b4bd011c2bbbf28d59275aa082ede1ff5 SearchIPCRouter: Invalidate cached connection ID on renderer crash
af445fb0a75d6811c0a8991becd79986e996d647 media: Add string for enabling protected content
d71997cab809617592987ba993d0261cdde19a9a Add usage counter for each value of webkit-user-modify
6004f9b3a66dce2ad170a521529d3414e26fb538 DevTools: Missing parenthesis on url in NetworkLog
585710fd6fe9ea1c4d919369e7f8a5063e785939 Fix crash when changing OS language while FRE is up.
4d68b33ea8398044d86aa76353e6f7de6880bad6 Change hover element to scrollbar's parent when hit test contains scrollbar
be1c7be7700037d82e0e92f66232795832f72101 Don't cache WindowAndroid and ChromeActivity.
4d68b33ea8398044d86aa76353e6f7de6880bad6 Change hover element to scrollbar's parent when hit test contains scrollbar
bf0aebdaabb28ca18c88568c7d92064d94a3046b [ios] Update panel after history item deletion
7496904fab33e2c38f8c6ec6cfad27ebd158c1f4 [NTP::SectionOrder] Track category index after it has been moved up.
04055dd2786e27cde8d210b68e7a726fab6d0c42 Scheduler: Enqueue non-JS-timer tasks into the unthrottled task runner
faaa2fd0a05f1622d9a8806da118d4f3b602e707 [Blink>Media] Allow autoplay muted on Android by default
45fbe12ea4207021df1cff4c49208a938c54aff5 [sync] Replace a CHECK for PathService::Get with error-handling code.
7c93dbe0d5fe53372321fa53368377ee6770405a Revert changes to download_prefs regarding PDF auto open.
6ce2738ac15a657f8716f6e837dfce3f9cdb8ee3 Reset provider_ on unregister.
38b110ec3981062d81c3e61a3a0508eee0f2f1c2 ChromeVox: Disable following a11y events after screen off alert is received
e44f64fb67cd9ea502dd4664e4c751c5dfc7b9c0 Fix bug with Android FRE spinner showing when going back to the page.
ce9ab459b6cce4f6d327b83ef49a8684a8839b9b Record UMA histogram when Form-Not-Secure warning is shown
b56160fa32a999a2b88a7987ca84efe277defe5a Add counters for clicking on Form-Not-Secure warnings
0cd28be01ef0b1f4a51d2806c4601264a53d4c53 Add HTTP-specific metrics for password forms
4ff2bf0b92b736c30c01f584ee41fd10f19f63a8 Read NQE prefs to network quality store under all cases
9d8e815220afc20f198f0b513cb46edc2d823186 DevTools: restore the style of expandable titles on console object previews
5fdae6fa2582b407ad6281c152e5cb6f7b95a391 Fix the size of omnibox suggestion answers.
10d16356dad16ba47a8b708d45aad2f291dabfff NQE: Do not record correlation if metric is missing
c853dbe32cdd2b2a6699593fe8e06879568bc6db [Autofill] Use user's email for converted wallet addresses
a008362df6fcd4e9114f534ca38d4bb8dc26f1e8 Remove special sync status message for child accounts.
46b74d13a37ebcc14f00a17a3583ed31ea157071 Update Chrome settings UI for child accounts.
c853dbe32cdd2b2a6699593fe8e06879568bc6db [Autofill] Use user's email for converted wallet addresses.
ed255a47bc7dddd9605ca08109fa396485984dc8 Implemented the conversion logic from Wallet addresses to local Autofill profiles:
b0a34cb69bdca47cfe3dd3d1cead96446391dcbf [Media>UI] Reset media metadata when navigating away
6004a36c9e965eedc2b83f2e034bc06afa9dd616 Track the original opener of a webcontents so we can rely on it for popups
c7dda59dad207a6642d7a201e7ed088f3e8bad92 Removed investigation CHECK from didCommitNavigation.
d8033dadec98c7c2f4c407f89c128b166b5922ef [DIAL] Break circular dependency in DialFetchDeviceDescriptionFunction.
a7140be113cefff6413d285e9bce20b513f52686 [Mac] Fullscreen layout issues
f2026d03c0b50a54ca5d4845edb64f65a49deff7 Make External per-window rather than a static Persistent.
13df10e53b1b9bd701917c981206c31405ad6b2d Fix Safe Browsing cert reports URL

Cc: nhiroki@chromium.org
Hiroki, do you think this regression might be related to your CL?
04055dd2786e27cde8d210b68e7a726fab6d0c42 Scheduler: Enqueue non-JS-timer tasks into the unthrottled task runner

Cc: skyos...@chromium.org
+Sami
Cc: altimin@chromium.org tzik@chromium.org
Components: Blink>Scheduling
Hmm... it's likely. I suspect tasks in background tabs that were moved to the unthrottled/unsuspendable task queue by my change could delay the UI tasks in a foreground tab.

skyostil@, altimin@, tzik@: Any thoughts?
Right, I think we also have the same problem just in foreground tabs. The scheduler throttles and blocks timers for short periods to improve touch responsiveness, and by moving timers into the unsuspendable queue this becomes less effective.

Since this throttling is very brief (<100ms), it should be safe to do it for these kinds of tasks. Maybe we need a new kind of task queue or a separate kind of throttling policy that applies more broadly?
Owner: nhiroki@chromium.org
Status: Assigned (was: Untriaged)
nhiroki@ Can you own driving a solution skyostil@ indicated?
Labels: -Pri-3 Pri-1
bumping priority as this is a 20% regression.
Owner: altimin@chromium.org
altimin@, do you think you could own this? I feel this could be conflict with your suspendable queue work (was it reverted...?)
Will do.

Sami, I think that the following structure will work:
timer < loading < suspendable < unthrottled, where suspendable tasks can be throttled for a short periods of time for touch responsiveness. WDYT?
Sounds good. We might also want to come up new names for the "short" and "long" throttling policies.
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cb434223a4b462abf2b9f9123646137250b3c2d9

commit cb434223a4b462abf2b9f9123646137250b3c2d9
Author: altimin <altimin@chromium.org>
Date: Wed Apr 05 20:45:32 2017

[scheduler] Add suspendable task queue.

Add suspendable task queue to avoid running (and cancelling) callbacks
when execution context is suspended.

R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org
CC=nhiroki@chromium.org,tzik@chromium.org
BUG= 704939 ,  702160 

Review-Url: https://codereview.chromium.org/2754673002
Cr-Original-Commit-Position: refs/heads/master@{#459787}
Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8f5016b59ce18
Review-Url: https://codereview.chromium.org/2754673002
Cr-Commit-Position: refs/heads/master@{#462208}

[modify] https://crrev.com/cb434223a4b462abf2b9f9123646137250b3c2d9/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
[modify] https://crrev.com/cb434223a4b462abf2b9f9123646137250b3c2d9/third_party/WebKit/Source/core/loader/EmptyClients.cpp
[modify] https://crrev.com/cb434223a4b462abf2b9f9123646137250b3c2d9/third_party/WebKit/Source/platform/WebFrameScheduler.h
[modify] https://crrev.com/cb434223a4b462abf2b9f9123646137250b3c2d9/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
[modify] https://crrev.com/cb434223a4b462abf2b9f9123646137250b3c2d9/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h
[modify] https://crrev.com/cb434223a4b462abf2b9f9123646137250b3c2d9/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl_unittest.cc

The change that was landed no Apr 5th days ago doesn't seem to be moving the Canary that much. Do we need more time to for to see the effect maybe in a more popular channel like Dev?

https://uma.googleplex.com/p/chrome/timeline_v2?sid=fe310e9e9476057aee02992a0453883a

This is not fixed yet, I'm working on it. I'll write in this thread after fixing it.
Any update on this?
Project Member

Comment 16 by bugdroid1@chromium.org, May 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f39f12be44e4d35eb25c7a58b1895c751da723f5

commit f39f12be44e4d35eb25c7a58b1895c751da723f5
Author: altimin <altimin@chromium.org>
Date: Tue May 30 17:53:12 2017

[scheduler] Add general task queue to fix touch latency regression.

Every problem can be solved by adding an additional task queue, except
for the problem of too many task queues.

This should fix touch latency regression which a) is considered timer
task queue from renderer scheduler point of view and can be blocked
during touch move if expensive b) is not throttled or suspended,
therefore not breaking any other use cases.

This is a temporary fix and all tasks should be moved to suspendable or
unthrottled task queues.

NOTE: this patch temporary disables background throttling of the default timer queue because otherwise suspendable tasks would be throttled too.

R=skyostil@chromium.org,haraken@chromium.org
CC=tdresser@chromium.org,nzolghadr@chromium.org

BUG= 704939 

Review-Url: https://codereview.chromium.org/2895673002
Cr-Commit-Position: refs/heads/master@{#475591}

[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/core/loader/EmptyClients.cpp
[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/platform/WebFrameScheduler.h
[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h
[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl_unittest.cc
[modify] https://crrev.com/f39f12be44e4d35eb25c7a58b1895c751da723f5/third_party/WebKit/Source/web/tests/scheduler/ThrottlingTest.cpp

altimin@ does you change affect anything on Windows differently by any chance? We are seeing a regression in Canary which includes your change:

https://uma.googleplex.com/timeline_v2?sid=a31966e92ce32727ebe0025709a89c91
Also there is no improvement seen across any other platforms in the range that your CL landed.
No, my patch doesn't have any windows-specific logic. Are there any touch gesture Windows-specific settings?

At the moment I'm looking into  crbug.com/729012 , which may be related to things we are seeing here.
I was digging into this regression a little more and found that it might not have been a merge to the branch. I looked at one specific device which the regression was obvious (Nexus5x):
https://uma.googleplex.com/timeline_v2?sid=06d293e870c24b0ed83158052dc82410

The regression does seemed to happen in Canary and after a little while in Dev and then in Beta. The range of the change in Canary is in https://chromium.googlesource.com/chromium/src/+log/57.0.2957.0..57.0.2958.0?n=10000

Also UMA metric by Chrome Canary version shows this as well:
https://uma.googleplex.com/timeline_v2?sid=2e46b13f0bba7275b178bb3817afb612

But that seems to be a big list even though it is only for two consecutive Canary data. Anything jumps up to your eyes, Tim?

Cc: flackr@chromium.org
What is our action item regarding this one? It is a P1 that has been untouched since June.
Wasn't this the one that we investigated with reverting (disabling) Lan's change and also you fixed a bug in timestamps at the same time and it recovered. Then I guess I forgot to close it at the time.
Status: WontFix (was: Assigned)
I'm tempted to close this as WontFix. We'll be working on UMA/UKM metrics to get a better understanding of touch performance and get more actionability.

Sign in to add a comment