372197 - regression: performance with large itemsets

Status: RESOLVED WONTFIX

(Core Graveyard :: XForms, defect)

Description

[aaronr]

Somewhere along the way, I assume due to the native widget rewrite in selects, we re-introduced a performance problem.  In bug 346325 we had greatly decreased the load time for a testcase with 30 selects and 50 items in each (done using itemsets).  Now we are back to where we were before that fix went in.  Load time in debug is about 66 seconds.  We need to get it down to about 10 seconds to get back to where we were when we shipped 0.7.0.1.

Testcase: https://bugzilla.mozilla.org/attachment.cgi?id=231124

Comment 1

[aaronr]

gotta be the new mutation handlers.  We are getting killed when we are cloning the templates into the anonymous content, especially the AppendChild here inside of nsXFormsItemSetElement::Refresh()

[...]

    // Clone the template content under the item
    for (PRUint32 j = 0; j < templateNodeCount; ++j) {
      templateNodes->Item(j, getter_AddRefs(templateNode));
      templateNode->CloneNode(PR_TRUE, getter_AddRefs(cloneNode));
      contextContainer->AppendChild(cloneNode, getter_AddRefs(templateNode));
    }

[...]

Alex, any optimizations we can make?  I love the way we now tie in the item with the native element directly (i.e. xf:item with xhtml:option), but this is killing us.  Like any way to do it the 'old way' during refresh, but still maintain the item label contents via mutation events when we aren't refreshing the itemset?

Comment 2

[alexander :surkov (:asurkov)]

The truth optimization is to stop use native widget approach ;). But if we stop this then we can stop this only on trunk of course. XHTML uses html:select for compact select/select1 elements. XUL uses: xul:listbox for compact select/select1 elements and xul:menulist for minimal select1.

1) XUL's compact select/select that use xul:listbox. I think xul:richlistbox is ready to for us.
2) XUL's minimal select1 that use xul:menulist. I don't know what we can do here
3) XHTML compact select/select1 that use html:select. See below.

All that we need is to create layout objects like css frame constructor makes it
(http://lxr.mozilla.org/mozilla/source/layout/base/nsCSSFrameConstructor.cpp#4950). The question about approach how exactly to do it is unclear with me. The thing we should do is to implement XHTML interfaces for xforms elements. Also we should add minor fixes for example at http://lxr.mozilla.org/mozilla/source/layout/forms/nsListControlFrame.cpp#445
Probably we should change something to allow xf:itemset inside list control frame.

Comment 3

[alexander :surkov (:asurkov)]

Though I just finished regorganization of native widget approach of selects and I spent a lot time for this but I think it's right time to take decision to stop it. It will bring a lot code fixes some of them are listed in bug 344685.

The other thing we can try for compact select/select1 is to use xul:richlistbox. If it works fine in XHTML then I believe it's not hard to extend richlistbox to allow hierarchy.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #1)

> Alex, any optimizations we can make?  I love the way we now tie in the item
> with the native element directly (i.e. xf:item with xhtml:option), but this is
> killing us.

We got awfull many 'DOMNodeInsertion'. If it the problem is our code is executed so many rather than speed of mutation events then we may improve it if we add bindings for item, choices and will be listen events there.

>  Like any way to do it the 'old way' during refresh, but still
> maintain the item label contents via mutation events when we aren't refreshing
> the itemset?
> 

Do you mean to do full refresh (remove all items and then add items)? If yes, then it will be regression too (I mean focus, tabnavigation) issue.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> The other thing we can try for compact select/select1 is to use
> xul:richlistbox. If it works fine in XHTML then I believe it's not hard to
> extend richlistbox to allow hierarchy.
> 

The problem is we can't inherit from our select-base widget and from richlistbox simultaneously. If we'll got idea how to get round then the approach should work.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: rough idea for XUL

Comment 7

[aaronr]

Attachment: testcase2

This testcase shows that we initially set the select1 as out-of-range and then set it as in range (as it should be).  That is because the select1 contains an itemset and our items are being added via DOM mutation events so the XBL isn't attaching until well after we bind the list of controls.  The select1 will be out of range since the bound value can't be found amongst the items (because none of the items from the itemset are there yet).

A fix for this bug should fix this testcase, I think.

Comment 8

[aaronr]

moving this out to 0.9 timeframe.  Whatever we do to fix this will be semi-involved and will probably cause a few regressions.  So I'd rather not rush this into 0.8 which I know lots of people are waiting for.

Comment 9

[alexander :surkov (:asurkov)]

Mike, why did you marked this bug as incomplete?

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Because it's an XForms bug, which I believed was no longer supported (release notes last updated in 2008, this bug last touched in 2007).  Did I err?

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I did err, though once a regression is 3 years old one wonders if it's really still considered a regression!  Apologies nonetheless.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

RIP xforms