Bug 76876 - Make elements that don't have attributes smaller.
Summary: Make elements that don't have attributes smaller.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on: 76930 77001 77304
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-23 16:21 PST by Andreas Kling
Modified: 2012-01-31 23:36 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.25 KB, patch)
2012-01-23 16:28 PST, Andreas Kling
koivisto: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (8.25 KB, patch)
2012-01-31 20:34 PST, Andreas Kling
sam: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>