Last Comment Bug 372197 - regression: performance with large itemsets
: regression: performance with large itemsets
Status: RESOLVED WONTFIX
: perf, regression
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal with 2 votes (vote)
: ---
Assigned To: xforms
: Stephen Pride
Mentors:
https://bugzilla.mozilla.org/attachme...
Depends on:
Blocks: 376307
  Show dependency treegraph
 
Reported: 2007-02-28 16:47 PST by aaronr
Modified: 2016-07-15 14:46 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
rough idea for XUL (8.12 KB, patch)
2007-02-28 23:46 PST, alexander :surkov
no flags Details | Diff | Splinter Review
testcase2 (2.70 KB, application/xhtml+xml)
2007-03-16 15:02 PDT, aaronr
no flags Details

Description aaronr 2007-02-28 16:47:23 PST
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 2007-02-28 17:32:25 PST
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 2007-02-28 18:57:21 PST
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 2007-02-28 19:13:32 PST
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 2007-02-28 19:19:30 PST
(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 2007-02-28 21:13:08 PST
(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 2007-02-28 23:46:23 PST
Created attachment 256889 [details] [diff] [review]
rough idea for XUL
Comment 7 aaronr 2007-03-16 15:02:27 PDT
Created attachment 258846 [details]
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 2007-04-02 18:07:36 PDT
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 2010-11-03 00:19:26 PDT
Mike, why did you marked this bug as incomplete?
Comment 10 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-11-03 00:22:43 PDT
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) 2010-11-03 00:25:07 PDT
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] 2016-02-04 12:21:15 PST
RIP xforms

Note You need to log in before you can comment on or make changes to this bug.