Bug 88059 - REGRESSION(r109729) [Form] Rendering of select/optgroup/option combination is too slow.
Summary: REGRESSION(r109729) [Form] Rendering of select/optgroup/option combination is...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on: 88084
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-01 00:56 PDT by yosin
Modified: 2012-06-06 20:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch 1 (2.17 KB, patch)
2012-06-01 01:42 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (2.08 KB, patch)
2012-06-01 01:44 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (2.15 KB, patch)
2012-06-06 03:30 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-06-01 00:56:50 PDT
This bug is imported from http://crbug.com/129334

Slowness is brought by StyleResolver::locateSharedStyle. When style object is shared among optgroup element, StyleResolver::locateSharedStyle checks option elements before it in document order.

For example, if we have following HTML:

<select>
  <optgroup label="1"> <option value="1-1>1-1</option> ... x 100 </optgroup>
  <optgroup label="2"> <option value="2-1>2-1</option> ... x 100 </optgroup>
  ...
  <optgroup label="10"> <option value="2-1>2-1</option> ... x 100 </optgroup>
</select>

locateSharedStyle checks last 10 option elements in optgroup[label="9" to label="1"] for option elements in optgroup[label="10"] by

            if (currentNode->renderStyle() == parentStyle && currentNode->lastChild()) {
                // Adjust for unused reserved tries.
                visitedNodeCount -= cStyleSearchThreshold - subcount;
                return currentNode->lastChild();
            }

Before r109729, style object isn't shared for optgroup element by:

bool StyleResolver::canShareStyleWithControl(StyledElement* element) const
{
    HTMLInputElement* thisInputElement = element->toInputElement();
    HTMLInputElement* otherInputElement = m_element->toInputElement();

    if (!thisInputElement || !otherInputElement)
        return false;

Because, "optgroup" element isn't "input" element.

BTW, canShareStyleWithControl should be smarter than non-input element to reduce memory usage for RenderStyle object.
Comment 1 yosin 2012-06-01 01:42:47 PDT
Created attachment 145241 [details]
Patch 1
Comment 2 yosin 2012-06-01 01:44:18 PDT
Created attachment 145242 [details]
Patch 2
Comment 3 yosin 2012-06-01 02:15:36 PDT
Comment on attachment 145242 [details]
Patch 2

Could you review this patch?
Thanks in advance.
Comment 4 Kent Tamura 2012-06-01 03:32:03 PDT
Comment on attachment 145242 [details]
Patch 2

Looks ok.
Comment 5 WebKit Review Bot 2012-06-01 04:53:27 PDT
Comment on attachment 145242 [details]
Patch 2

Clearing flags on attachment: 145242

Committed r119213: <http://trac.webkit.org/changeset/119213>
Comment 6 WebKit Review Bot 2012-06-01 04:53:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Zoltan Arvai 2012-06-01 05:33:10 PDT
Layout tests failing on Qt and GTK after r119213

fast/forms/select/menulist-disabled-option.html:

Three and Two changed to One and Two on the select box.


fast/css/text-transform-select.html:

--- /ramdisk/qt-linux-release/build/layout-test-results/fast/css/text-transform-select-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/fast/css/text-transform-select-actual.txt 
@@ -9,8 +9,8 @@
       RenderBlock {DIV} at (0,21) size 768x96
         RenderMenuList {SELECT} at (2,68) size 88x26 [bgcolor=#FFFFFF]
           RenderBlock (anonymous) at (4,2) size 60x21
-            RenderText at (0,0) size 53x21
-              text run at (0,0) width 53: "HELLO"
+            RenderText at (0,0) size 45x21
+              text run at (0,0) width 45: "heLLo"
         RenderText {#text} at (92,70) size 4x21
           text run at (92,70) width 4: " "
         RenderListBox {SELECT} at (98,2) size 82x89 [bgcolor=#FFFFFF] [border: (1px inset #808080)]
@@ -23,8 +23,8 @@
       RenderBlock {DIV} at (0,117) size 768x96
         RenderMenuList {SELECT} at (2,68) size 82x26 [bgcolor=#FFFFFF]
           RenderBlock (anonymous) at (4,2) size 54x21
-            RenderText at (0,0) size 48x21
-              text run at (0,0) width 48: "HeLLo"
+            RenderText at (0,0) size 45x21
+              text run at (0,0) width 45: "heLLo"
         RenderText {#text} at (86,70) size 4x21
           text run at (86,70) width 4: " "
         RenderListBox {SELECT} at (92,2) size 76x89 [bgcolor=#FFFFFF] [border: (1px inset #808080)]
@@ -37,8 +37,8 @@
       RenderBlock {DIV} at (0,213) size 768x96
         RenderMenuList {SELECT} at (2,68) size 67x26 [bgcolor=#FFFFFF]
           RenderBlock (anonymous) at (4,2) size 39x21
-            RenderText at (0,0) size 35x21
-              text run at (0,0) width 35: "hello"
+            RenderText at (0,0) size 45x21
+              text run at (0,0) width 45: "heLLo"
         RenderText {#text} at (71,70) size 4x21
           text run at (71,70) width 4: " "
         RenderListBox {SELECT} at (77,2) size 61x89 [bgcolor=#FFFFFF] [border: (1px inset #808080)]
@@ -51,8 +51,8 @@
       RenderBlock {DIV} at (0,309) size 768x96
         RenderMenuList {SELECT} at (2,68) size 50x26 [bgcolor=#FFFFFF]
           RenderBlock (anonymous) at (4,2) size 22x21
-            RenderText at (0,0) size 22x21
-              text run at (0,0) width 22: "SS"
+            RenderText at (0,0) size 10x21
+              text run at (0,0) width 10: "\x{DF}"
         RenderText {#text} at (54,70) size 4x21
           text run at (54,70) width 4: " "
         RenderListBox {SELECT} at (60,2) size 44x89 [bgcolor=#FFFFFF] [border: (1px inset #808080)]
Comment 8 Csaba Osztrogonác 2012-06-01 06:03:55 PDT
and Lion and EFL too ... Could you guys fix it or should we roll it out?
Comment 9 WebKit Review Bot 2012-06-01 06:25:54 PDT
Re-opened since this is blocked by 88084
Comment 10 Stephen Chenney 2012-06-01 07:04:07 PDT
r119213 also broke Chromium tests:
fast/css/text-transform-select.html
fast/forms/select/menulist-disabled-option.html

It was rolled out.
Comment 11 Stephen Chenney 2012-06-01 07:26:57 PDT
The Chromium failures were on Mac and Win, not Linux.
Comment 12 yosin 2012-06-06 03:30:52 PDT
Created attachment 145979 [details]
Patch 3
Comment 13 yosin 2012-06-06 03:34:24 PDT
(In reply to comment #10)
> r119213 also broke Chromium tests:
> fast/css/text-transform-select.html
> fast/forms/select/menulist-disabled-option.html
> 
> It was rolled out.

I file a bug to share RenderStyle object for option and optgroup elements, bug 88405. A root causes are different in these test cases.
Comment 14 yosin 2012-06-06 18:25:20 PDT
Comment on attachment 145979 [details]
Patch 3

Could you review this patch?
Thanks in advance.
Comment 15 Kent Tamura 2012-06-06 18:26:53 PDT
Comment on attachment 145979 [details]
Patch 3

ok
Comment 16 WebKit Review Bot 2012-06-06 20:47:34 PDT
Comment on attachment 145979 [details]
Patch 3

Clearing flags on attachment: 145979

Committed r119672: <http://trac.webkit.org/changeset/119672>
Comment 17 WebKit Review Bot 2012-06-06 20:47:42 PDT
All reviewed patches have been landed.  Closing bug.