New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 899603 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Regression

Blocking:
issue 896748



Sign in to add a comment

Regression:Delay is seen in loading 'Manage people' overlay

Reported by vineetha...@etouch.net, Oct 29

Issue description

Chrome 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
 
ExpectedVideo.mp4
246 KB View Download
ActualVideo.mp4
285 KB View Download
Blocking: 896748
Cc: rbpotter@chromium.org dpa...@chromium.org
Labels: -Pri-1 Pri-2
Owner: ----
Status: Available (was: Assigned)
I am able to reproduce the difference in load time for the "manage people" window.
Cc: dfreedm@chromium.org
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.

Attaching trace.
slow_regexp.png
72.2 KB View Download
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).
no_regexp.png
108 KB View Download
md-user-manager-1540841077684.log
47.3 KB View Download
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.
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.
Owner: dpa...@chromium.org
Status: Started (was: Available)
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. 
 Issue 900543  has been merged into this issue.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Labels: TE-Verified-M72 TE-Verified-72.0.3599.0
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.
CanaryBehaviour.mp4
174 KB View Download

Sign in to add a comment