Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1929763002:
Update Polymer.IronValidatableBehavior (Closed)
|
Created:
4 years, 7 months ago by michaelpg Modified:
4 years, 7 months ago CC:
chromium-reviews, asanka, dbeam+watch-polymer_chromium.org, michaelpg+watch-polymer_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, dbeam+watch-downloads_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate Polymer.IronValidatableBehavior
IronValidatableBehavior used to create an IronMeta for every instance
of the behavior (e.g., every paper-dropdown-menu). It has been updated
to use a singleton IronMeta and noms@ is interested in seeing the effects
on performance.
NOPRESUBMIT=true # crisper.js
BUG=602896
R=dbeam@chromium.org
TBR=yawano@chromium.org # ui/file_manager
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6085f5c136540213174fdeb7529e10cc9ca81713
Cr-Commit-Position: refs/heads/master@{#392786}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : gyp #Patch Set 5 : rebase #
Messages
Total messages: 34 (15 generated)
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org, noms@chromium.org
michaelpg@chromium.org changed required reviewers: + dbeam@chromium.org
@noms: given the polytiming numbers at https://docs.google.com/spreadsheets/d/1vXrbdADx3GreDhtxAUQa6jakRBSEiP97oHZkM... would you expect this to have a non-negligible impact on Settings page load time? (this seems to *increase* our load time by like 1%)
Huh I would have expected the exact opposite. In my testing, elements got faster -- basically, all the old IronMeta values you were seeing should go to 0. What does polydev say, in comparison to polytiming? On Wed, Apr 27, 2016 at 7:58 PM <michaelpg@chromium.org> wrote: > Reviewers: *Dan Beam, noms (inactive) > CL: https://codereview.chromium.org/1929763002/ > > Message: > @noms: given the polytiming numbers at > > https://docs.google.com/spreadsheets/d/1vXrbdADx3GreDhtxAUQa6jakRBSEiP97oHZkM... > would you expect this to have a non-negligible impact on Settings > page load time? (this seems to *increase* our load time by like 1%) > > Description: > Update Polymer.IronValidatableBehavior > > NOPRESUBMIT=true # crisper.js > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+84, -41 lines): > M chrome/browser/resources/md_downloads/crisper.js > M > third_party/polymer/v1_0/components-chromium/iron-validatable-behavior/bower.json > M > third_party/polymer/v1_0/components-chromium/iron-validatable-behavior/iron-validatable-behavior-extracted.js > M third_party/polymer/v1_0/components_summary.txt > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
soooo am I supposed to lg a change to make us slower? what's actionable for me here?
On 2016/05/03 04:26:40, Dan Beam wrote: > soooo am I supposed to lg a change to make us slower? what's actionable for me > here? my guess is it has no significant impact one way or the other, but i won't submit it yet would you like me to close this until noms and i look at the polydev numbers and then re-open it?
On 2016/04/28 02:58:23, michaelpg wrote: > @noms: given the polytiming numbers at > https://docs.google.com/spreadsheets/d/1vXrbdADx3GreDhtxAUQa6jakRBSEiP97oHZkM... > would you expect this to have a non-negligible impact on Settings > page load time? (this seems to *increase* our load time by like 1%) on ToT, the % improvements are: Linux CrOS Basic 8.3% -1.7% Advanced 3.9% 4.4% The CrOS/Basic combination is the only build where this CL is slower, and not by much (within .4 standard deviation). The main difference between Linux and CrOS on that page is that CrOS has 56 paper-icon-buttons (linux has 3), and CrOS has 82 iron-icons (linux has 23). noms@ any reason the IronValidatable change might make paper-icon-button slower?
On 2016/05/04 03:43:44, michaelpg wrote: > On 2016/04/28 02:58:23, michaelpg wrote: > > @noms: given the polytiming numbers at > > > https://docs.google.com/spreadsheets/d/1vXrbdADx3GreDhtxAUQa6jakRBSEiP97oHZkM... > > would you expect this to have a non-negligible impact on Settings > > page load time? (this seems to *increase* our load time by like 1%) > > on ToT, the % improvements are: > > Linux CrOS > Basic 8.3% -1.7% > Advanced 3.9% 4.4% > > The CrOS/Basic combination is the only build where this CL is slower, and not by > much (within .4 standard deviation). > > The main difference between Linux and CrOS on that page is that CrOS has 56 > paper-icon-buttons (linux has 3), and CrOS has 82 iron-icons (linux has 23). > noms@ any reason the IronValidatable change might make paper-icon-button slower? The Internet section (CrOS Basic page only) is a ridiculously heavy section, so I'm willing to ignore those results entirely since we'll need to give that section some TLC anyway. Dan, care to review now?
Description was changed from ========== Update Polymer.IronValidatableBehavior NOPRESUBMIT=true # crisper.js ========== to ========== Update Polymer.IronValidatableBehavior NOPRESUBMIT=true # crisper.js CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
On 2016/05/04 04:01:20, michaelpg wrote: > On 2016/05/04 03:43:44, michaelpg wrote: > > On 2016/04/28 02:58:23, michaelpg wrote: > > > @noms: given the polytiming numbers at > > > > > > https://docs.google.com/spreadsheets/d/1vXrbdADx3GreDhtxAUQa6jakRBSEiP97oHZkM... > > > would you expect this to have a non-negligible impact on Settings > > > page load time? (this seems to *increase* our load time by like 1%) > > > > on ToT, the % improvements are: > > > > Linux CrOS > > Basic 8.3% -1.7% > > Advanced 3.9% 4.4% > > > > The CrOS/Basic combination is the only build where this CL is slower, and not > by > > much (within .4 standard deviation). > > > > The main difference between Linux and CrOS on that page is that CrOS has 56 > > paper-icon-buttons (linux has 3), and CrOS has 82 iron-icons (linux has 23). > > noms@ any reason the IronValidatable change might make paper-icon-button > slower? > > The Internet section (CrOS Basic page only) is a ridiculously heavy section, so > I'm willing > to ignore those results entirely since we'll need to give that section some TLC > anyway. > > Dan, care to review now? will when you fix that red "closure_compilation" box, sure ;)
On 2016/05/05 16:36:07, Dan Beam wrote: > On 2016/05/04 04:01:20, michaelpg wrote: > > On 2016/05/04 03:43:44, michaelpg wrote: > > > On 2016/04/28 02:58:23, michaelpg wrote: > > > > @noms: given the polytiming numbers at > > > > > > > > > > https://docs.google.com/spreadsheets/d/1vXrbdADx3GreDhtxAUQa6jakRBSEiP97oHZkM... > > > > would you expect this to have a non-negligible impact on Settings > > > > page load time? (this seems to *increase* our load time by like 1%) > > > > > > on ToT, the % improvements are: > > > > > > Linux CrOS > > > Basic 8.3% -1.7% > > > Advanced 3.9% 4.4% > > > > > > The CrOS/Basic combination is the only build where this CL is slower, and > not > > by > > > much (within .4 standard deviation). > > > > > > The main difference between Linux and CrOS on that page is that CrOS has 56 > > > paper-icon-buttons (linux has 3), and CrOS has 82 iron-icons (linux has 23). > > > noms@ any reason the IronValidatable change might make paper-icon-button > > slower? > > > > The Internet section (CrOS Basic page only) is a ridiculously heavy section, > so > > I'm willing > > to ignore those results entirely since we'll need to give that section some > TLC > > anyway. > > > > Dan, care to review now? > > will when you fix that red "closure_compilation" box, sure ;) Nice, if we had landed this a week ago that wouldn't have been caught. Opened https://github.com/PolymerElements/iron-validatable-behavior/pull/25 and will update when that's in.
compiles now
lgtm but please find a relevant bug or explain why we're rolling this one element, as I don't really understand why we are at this point
Description was changed from ========== Update Polymer.IronValidatableBehavior NOPRESUBMIT=true # crisper.js CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Update Polymer.IronValidatableBehavior NOPRESUBMIT=true # crisper.js BUG=602896 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
On 2016/05/10 05:44:22, Dan Beam wrote: > lgtm but please find a relevant bug or explain why we're rolling this one > element, as I don't really understand why we are at this point there's not much to it, it's just a (theoretically) significant optimization and I wanted to push this out without waiting for someone to roll all of Polymer. which makes less sense after prolonging it for two weeks, but, it works now, so whatever.
Description was changed from ========== Update Polymer.IronValidatableBehavior NOPRESUBMIT=true # crisper.js BUG=602896 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Update Polymer.IronValidatableBehavior IronValidatableBehavior used to create an IronMeta for every instance of the behavior (e.g., every paper-dropdown-menu). It has been updated to use a singleton IronMeta and noms@ is interested in seeing the effects on performance. NOPRESUBMIT=true # crisper.js BUG=602896 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929763002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929763002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
On 2016/05/10 06:05:51, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > closure_compilation on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...) @dbeam, halp! This compiles locally and I can confirm the IronMeta call looks correct, and is already included in IronValidatable's compiled_resources2.gyp.
michaelpg@chromium.org changed reviewers: + yawano@chromium.org
michaelpg@chromium.org changed required reviewers: + yawano@chromium.org
I believe this fixes the compile. yawano@ PTAL.
still lgtm (you can probably just TBR= the ui/file_manager part, IMO)
Description was changed from ========== Update Polymer.IronValidatableBehavior IronValidatableBehavior used to create an IronMeta for every instance of the behavior (e.g., every paper-dropdown-menu). It has been updated to use a singleton IronMeta and noms@ is interested in seeing the effects on performance. NOPRESUBMIT=true # crisper.js BUG=602896 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Update Polymer.IronValidatableBehavior IronValidatableBehavior used to create an IronMeta for every instance of the behavior (e.g., every paper-dropdown-menu). It has been updated to use a singleton IronMeta and noms@ is interested in seeing the effects on performance. NOPRESUBMIT=true # crisper.js BUG=602896 R=dbeam@chromium.org TBR=yawano@chromium.org # ui/file_manager CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1929763002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929763002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929763002/100001
Message was sent while issue was closed.
Description was changed from ========== Update Polymer.IronValidatableBehavior IronValidatableBehavior used to create an IronMeta for every instance of the behavior (e.g., every paper-dropdown-menu). It has been updated to use a singleton IronMeta and noms@ is interested in seeing the effects on performance. NOPRESUBMIT=true # crisper.js BUG=602896 R=dbeam@chromium.org TBR=yawano@chromium.org # ui/file_manager CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Update Polymer.IronValidatableBehavior IronValidatableBehavior used to create an IronMeta for every instance of the behavior (e.g., every paper-dropdown-menu). It has been updated to use a singleton IronMeta and noms@ is interested in seeing the effects on performance. NOPRESUBMIT=true # crisper.js BUG=602896 R=dbeam@chromium.org TBR=yawano@chromium.org # ui/file_manager CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Update Polymer.IronValidatableBehavior IronValidatableBehavior used to create an IronMeta for every instance of the behavior (e.g., every paper-dropdown-menu). It has been updated to use a singleton IronMeta and noms@ is interested in seeing the effects on performance. NOPRESUBMIT=true # crisper.js BUG=602896 R=dbeam@chromium.org TBR=yawano@chromium.org # ui/file_manager CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Update Polymer.IronValidatableBehavior IronValidatableBehavior used to create an IronMeta for every instance of the behavior (e.g., every paper-dropdown-menu). It has been updated to use a singleton IronMeta and noms@ is interested in seeing the effects on performance. NOPRESUBMIT=true # crisper.js BUG=602896 R=dbeam@chromium.org TBR=yawano@chromium.org # ui/file_manager CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6085f5c136540213174fdeb7529e10cc9ca81713 Cr-Commit-Position: refs/heads/master@{#392786} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6085f5c136540213174fdeb7529e10cc9ca81713 Cr-Commit-Position: refs/heads/master@{#392786} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
