Bug 76876

Summary: Make elements that don't have attributes smaller.
Product: WebKit Reporter: Andreas Kling <kling>
Component: DOMAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, koivisto, leviw, menard, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 76930, 77001, 77304    
Bug Blocks:    
Attachments:
Description Flags
Patch
koivisto: review+, webkit.review.bot: commit-queue-
Patch v2 sam: review+, webkit.review.bot: commit-queue-

Description Andreas Kling 2012-01-23 16:21:28 PST
Because you're worth it.
Comment 1 Andreas Kling 2012-01-23 16:28:24 PST
Created attachment 123652 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-23 16:31:53 PST
Attachment 123652 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/StyledElement.h:93:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2012-01-24 06:23:55 PST
Comment on attachment 123652 [details]
Patch

Attachment 123652 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11212804

New failing tests:
fast/css/getComputedStyle/computed-style-border-image.html
Comment 4 Antti Koivisto 2012-01-24 06:56:43 PST
Comment on attachment 123652 [details]
Patch

It would be good to add some comments to NamedNodeMap about the refactoring plans. The class is getting increasingly confusing as a place to hang random stuff.
Comment 5 Andreas Kling 2012-01-24 07:42:19 PST
Committed r105738: <http://trac.webkit.org/changeset/105738>
Comment 6 Csaba Osztrogon√°c 2012-01-24 08:57:09 PST
Reopen, because it made fast/css/getComputedStyle/computed-style-border-image.html crash (at least) on Qt. Could you check what happened?
Comment 7 Andreas Kling 2012-01-24 10:57:00 PST
(In reply to comment #6)
> Reopen, because it made fast/css/getComputedStyle/computed-style-border-image.html crash (at least) on Qt. Could you check what happened?

Naturally. Here's the stack:

#0  WebCore::CSSValue::cssText (this=0x0) at CSSValue.h:153
#1  0x013fddbd in WebCore::CSSValueList::customCssText () at /Users/andreaskling/src/WebKit/Source/WebCore/css/CSSValueList.cpp:133
#2  0x013fcd0a in WebCore::CSSValue::cssText (this=<value temporarily unavailable, due to optimizations>) at /Users/andreaskling/src/WebKit/Source/WebCore/css/CSSValue.cpp:145
#3  0x013b1066 in WebCore::CSSProperty::cssText (this=0x2fb5990) at /Users/andreaskling/src/WebKit/Source/WebCore/css/CSSProperty.cpp:32
#4  0x01386ff1 in WebCore::CSSMutableStyleDeclaration::cssText () at /Users/andreaskling/src/WebKit/Source/WebCore/css/CSSMutableStyleDeclaration.cpp:862
#5  0x01c3abfa in WebCore::StyledElement::updateStyleAttribute (this=0x2fb5f20) at /Users/andreaskling/src/WebKit/Source/WebCore/dom/StyledElement.cpp:116
#6  0x012e3da6 in WebCore::StyledElement::ensureInlineStyleDecl (this=0x2fb5f20) at Element.h:495
#7  0x017cacae in WebCore::jsElementStyle (exec=0xc8ea1a0, slotBase={u = {asInt64 = -21284731472, asDouble = -nan(0xffffb0b54c5b0), asBits = {payload = 190105008, tag = -5}}}) at /Users/andreaskling/Source/Apple/Build/Release/DerivedSources/WebCore/JSElement.cpp:270

I wonder why we have null CSSValues in the CSSValueList here, this smells fishy.
Comment 8 Andreas Kling 2012-01-24 11:14:33 PST
Looks like I exposed a bug that was introduced in <http://trac.webkit.org/changeset/105502>
Comment 9 Andreas Kling 2012-01-26 06:56:45 PST
Committed r105999: <http://trac.webkit.org/changeset/105999>
Comment 10 Levi Weintraub 2012-01-27 15:50:30 PST
(In reply to comment #9)
> Committed r105999: <http://trac.webkit.org/changeset/105999>

I'm worried that this caused a performance regression. Here is the graph: http://build.chromium.org/f/chromium/perf/mac-release-10.5/dhtml/report.html?history=150&rev=-1

I'm running this test with all Chromium changes up to the latest WebKit roll, which included only these revisions: http://trac.webkit.org/log/?rev=106007&stop_rev=105998&verbose=on
Comment 11 Levi Weintraub 2012-01-28 19:19:23 PST
We did a perf test right before this change, and the regression wasn't present. I'm going to rever this tomorrow morning and try again just for confirmation. Let me know if you have an objection.
Comment 12 Andreas Kling 2012-01-29 06:26:59 PST
(In reply to comment #11)
> We did a perf test right before this change, and the regression wasn't present. I'm going to rever this tomorrow morning and try again just for confirmation. Let me know if you have an objection.

Feel free to roll out, I won't have a chance to look until tomorrow anyway. :/
Comment 13 Levi Weintraub 2012-01-29 15:58:29 PST
(In reply to comment #12)
> Feel free to roll out, I won't have a chance to look until tomorrow anyway. :/

Thanks! Just did. We'll know by tomorrow if this was the root cause. Sorry for the bother.
Comment 14 Levi Weintraub 2012-01-30 12:10:54 PST
(In reply to comment #13)
> Thanks! Just did. We'll know by tomorrow if this was the root cause. Sorry for the bother.

http://trac.webkit.org/changeset/106201 was the rollout, and it did fix this performance regression.
Comment 15 Andreas Kling 2012-01-31 20:28:29 PST
Reopening to roll back in.
Comment 16 Andreas Kling 2012-01-31 20:34:50 PST
Created attachment 124878 [details]
Patch v2

AFAICT the performance regression was introduced by causing us to unnecessarily serialize the inline style declaration into the element's 'style' attribute a lot more often. This happened because I used Element::attributes(false) to force creation of the element's attribute map. This time around, I've added a single-purpose ensureAttributeMap() for that instead.
Comment 17 WebKit Review Bot 2012-01-31 21:54:55 PST
Comment on attachment 124878 [details]
Patch v2

Attachment 124878 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11386328

New failing tests:
fast/doctypes/002.html
Comment 18 Andreas Kling 2012-01-31 23:36:33 PST
Committed r106435: <http://trac.webkit.org/changeset/106435>