Issue metadata
Sign in to add a comment
|
95.3%-96.6% regression in blink_perf.bindings at 393822:393848 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
May 19 2016
The range is https://chromium.googlesource.com/chromium/src/+log/0537a6d11bf2ebefcf90fbfec7081e37ef78081a%5E..2768accc154d067704fb501967de68cba9fa4c3b?pretty=fuller The most likely culprit seems to be: crrev.com/393827 - bindings: Supports [SameObject] extended attribute. yukishiino@ can you please take a look.
,
May 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fdd68c2f872ba56475500d80dcf02b1aeb97cbc4 commit fdd68c2f872ba56475500d80dcf02b1aeb97cbc4 Author: yukishiino <yukishiino@chromium.org> Date: Fri May 20 13:34:11 2016 Revert of bindings: Supports [SameObject] extended attribute. (patchset #2 id:20001 of https://codereview.chromium.org/1980553002/ ) Reason for revert: This CL hits terrible performance regressions. Original issue's description: > bindings: Supports [SameObject] extended attribute. > > Syntactically we've supported [SameObject] extended attribute, > but we've not had any implementation for it. > > This CL stores the first returned value in the holder's > private value, and returns it for the second time. > > BUG= 462913 > > Committed: https://crrev.com/40d19a73c02f0185e5a49c4761860762896dec28 > Cr-Commit-Position: refs/heads/master@{#393827} TBR=bashi@chromium.org,haraken@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 462913 , 613172 , 613170 Review-Url: https://codereview.chromium.org/2000643002 Cr-Commit-Position: refs/heads/master@{#395064} [modify] https://crrev.com/fdd68c2f872ba56475500d80dcf02b1aeb97cbc4/third_party/WebKit/LayoutTests/fast/dom/gc-9-expected.txt [modify] https://crrev.com/fdd68c2f872ba56475500d80dcf02b1aeb97cbc4/third_party/WebKit/Source/bindings/scripts/v8_attributes.py [modify] https://crrev.com/fdd68c2f872ba56475500d80dcf02b1aeb97cbc4/third_party/WebKit/Source/bindings/templates/attributes.cpp [modify] https://crrev.com/fdd68c2f872ba56475500d80dcf02b1aeb97cbc4/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp [modify] https://crrev.com/fdd68c2f872ba56475500d80dcf02b1aeb97cbc4/third_party/WebKit/Source/core/css/CSSImportRule.cpp [modify] https://crrev.com/fdd68c2f872ba56475500d80dcf02b1aeb97cbc4/third_party/WebKit/Source/core/css/CSSImportRule.idl
,
May 23 2016
,
Jul 29 2016
It turned out that https://crrev.com/2000643002 was committed in M53, but not in M52, and M52 still has the performance regression. Let me request a merge of the CL to M52. I'm very sorry that I haven't noticed it and I haven't merged the CL much earlier.
,
Jul 29 2016
[Automated comment] Request affecting a post-stable build (M52), manual review required.
,
Jul 29 2016
M52 is already in Stable for Desktop and bar is VERY high. We're going to have M52 refresh Stable release next week and we can take this change in ONLY if it is critical, well baked/verified in Canary/dev/beta and very safe to merge. Please confirm this. Thank you. Also is this change applicable to all OS or any specific OS?
,
Jul 29 2016
haraken@, majidvp@ and I discussed briefly. This was only detected by a single benchmark, and this is unlikely to impact real world sites right now (though it may have a more pronounced affect on libraries such as Polymer). Given that M52 is already rolling out to stable and that all new changes pose risk, we decided it is best not to revert. Thus merge rejected and marking back as fixed. Please take care to merge fixes back to release branches sooner in the future (though thank you for noticing and attempting to follow up when you did notice). |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by majidvp@chromium.org
, May 19 2016