Regression:Delay is seen in loading 'Manage people' overlay
Reported by
vineetha...@etouch.net,
Oct 29
|
|||||
Issue descriptionChrome Version:72.0.3595.0 (Official Build) Revision 8f9f67d1faa88f5dece06904791789cf64c068be-refs/branch-heads/3595@{#1}(32/64-bit) OS: Windows(7,8,8.1,10), Mac(10.13.1, 10.13.6, 10.14.1) and Linux(14.04 LTS) OS What steps will reproduce the problem? 1. Fresh launch chrome, click on Avatar icon to the RHS of omnibox. 2. Click on 'Manage people' and observe. Actual Result : Delay is seen in loading 'Manage people' overlay. Expected Result: Delay should not be seen in loading 'Manage people' overlay. This is a regression issue broken in ‘M-72’ and and below is the bisect info: Good Build : 72.0.3583.0 Bad Build : 72.0.3584.0 Chromium bisect info: You are probably looking for a change made after 600223 (known good), but no later than 600228 (first known bad). CHANGE-LOG URL: https://chromium.googlesource.com/chromium/src/+log/62f2f8f209a82db0dd5a311eb4f844bddb75c98e..631a939b2b9a02a05cd1db367c7ea7fb17d14f50 Suspecting: https://chromium.googlesource.com/chromium/src/+/177774d57a744b75630cba5507c3bf1ed419aea3 @dpapad: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner. Note: Tried per revision bisect on Windows ,Mac and Linux OS but unable to perform the same since getting "RuntimeError: We don't have enough builds to bisect" error hence, providing chromium bisect. Thank You
,
Oct 29
At first glance, it seems that Polymer2 is running a very slow Regex within replaceDirInCssText() that takes 700ms, which in turn is called once for every call to Polymer() function. Will investigate further, since I still don't have a good explanation of why this only happens for md-user-manager, but suspecting that is because in other Polymerized WebUI pages we don't use CSS files anymore in favor of <style> modules.
,
Oct 29
Attaching trace.
,
Oct 29
FYI, verified that replaceDirInCssText() is the cause of the slowness. If I comment it out within Polymer itself, the page loads very fast (~500ms), see attachment. Note that md-user-manager includes several inlined PNG images in its CSS, which I believe is why it take so long for the Regex within replaceDirInCssText() to process it (unlike Settings or other pages).
,
Oct 29
we don't currently use :dir() in webui's code (AFAICT) and I'd be happy to create a patch to remove this in the meantime (w/ a presubmit that disables folks from mistakenly adding uses).
as i mentioned to dpapad@ over chat: if we get away from the legacy Polymer({...}) syntax, I think we'd have more control / ability to opt into this mixin (right now it's hard to opt out). That's still a ways away, though.
,
Oct 29
WIP CL https://chromium-review.googlesource.com/c/chromium/src/+/1306515, which removes any usage of DirMixin from the final polymer2 bundle under third_party/polymer.
,
Oct 29
,
Oct 30
After inspecting the code found two usages of :dir(). 1) In https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chromium/paper-input/paper-input-char-counter.html?l=46, which can be removed because this element is not used anymore (last usage from paper-input is removed at https://chromium-review.googlesource.com/c/chromium/src/+/1306346) 2) In https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chromium/paper-input/paper-input-container.html?l=257,258. It's still not clear to me whether this makes use of the :dir() rule, will investigate further. Either way, this is supposed to be migrated to cr-input tracked by issue 856310. I'll remove 1 first, and then investigate 2 a bit more. After both of these are removed, it seems safe to go with the CL at #6 to dir-mixin.html from Polymer 2.
,
Oct 31
Re #8: 1) is being addressed at https://chromium-review.googlesource.com/c/chromium/src/+/1309226. 2) I posted the wrong link previously. The Chromium usage is at https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/gaia_input.html?l=8.
,
Oct 31
Issue 900543 has been merged into this issue.
,
Nov 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9d00b88f1ffbb8219bf78ff12aa1077b7e16af15 commit 9d00b88f1ffbb8219bf78ff12aa1077b7e16af15 Author: dpapad <dpapad@chromium.org> Date: Thu Nov 01 23:51:29 2018 WebUI Polymer 2: Don't include dir-mixin.html in the final bundle. Locally modify Polymer 2 to exclude dir-mixin.html, since it is unnecessary currently (we don't directly* use any :dir() CSS rules), and also causes a performance regression (more notice-able in chrome://md-user-manager), due to a Regex running over a fairly large amount of text (because of inlined PNG background images). *The only usage of :dir() was in paper-input-container.html, which is still used by ChromeOS <gaia-input> element. Modified <paper-input-container> to directly use :host-context instead. Also :dir() is not implemented in Chrome, so we should not be using it in our own code anyway. Ideally the usage of this mixin should be exposed as a Polymer option, so that we don't need to manually modify our local Polymer 2 copy. Bug: 899603 Change-Id: Ia20535fb2adb2c9a6bebf3046e4405a4ca31af6c Reviewed-on: https://chromium-review.googlesource.com/c/1306515 Reviewed-by: Scott Chen <scottchen@chromium.org> Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org> Cr-Commit-Position: refs/heads/master@{#604759} [modify] https://crrev.com/9d00b88f1ffbb8219bf78ff12aa1077b7e16af15/third_party/polymer/README.chromium [modify] https://crrev.com/9d00b88f1ffbb8219bf78ff12aa1077b7e16af15/third_party/polymer/v1_0/chromium.patch [modify] https://crrev.com/9d00b88f1ffbb8219bf78ff12aa1077b7e16af15/third_party/polymer/v1_0/components-chromium/paper-input/paper-input-container.html [modify] https://crrev.com/9d00b88f1ffbb8219bf78ff12aa1077b7e16af15/third_party/polymer/v1_0/components-chromium/polymer2/polymer-extracted.js [modify] https://crrev.com/9d00b88f1ffbb8219bf78ff12aa1077b7e16af15/third_party/polymer/v1_0/reproduce.sh
,
Nov 2
,
Nov 2
Update : Rechecked the above issue on Windows(7,8,8.1,10), Mac(10.13.1 , 10.13.6, 10.14.1) and Linux(14.04 LTS)OS with latest Canary version #72.0.3599.0 and the issue is fixed. Kindly refer the attached screen cast. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by dpa...@chromium.org
, Oct 29Cc: rbpotter@chromium.org dpa...@chromium.org
Labels: -Pri-1 Pri-2
Owner: ----
Status: Available (was: Assigned)