New issue
Advanced search Search tips

Issue 613172 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

95.3%-96.6% regression in blink_perf.bindings at 393822:393848

Project Member Reported by majidvp@chromium.org, May 19 2016

Issue description

See the link to graphs below.
 
Owner: yukishiino@chromium.org
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.
Project Member

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

Components: Blink>Bindings
Status: Fixed (was: Assigned)
Labels: Merge-Request-52
Status: Started (was: Fixed)
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.

Comment 6 by dimu@chromium.org, Jul 29 2016

Labels: -Merge-Request-52 Merge-Review-52 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M52), manual review required.

Comment 7 by gov...@chromium.org, 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?
Labels: -Merge-Review-52
Status: Fixed (was: Started)
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