regression: performance with large itemsets

RESOLVED WONTFIX

Status

Core Graveyard
XForms
RESOLVED WONTFIX
10 years ago
11 months ago

People

(Reporter: aaronr, Unassigned)

Tracking

({perf, regression})

Trunk
x86
All
perf, regression

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Updated

10 years ago
Blocks: 353738

Updated

10 years ago
Keywords: perf, regression
(Reporter)

Comment 1

10 years ago
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

10 years ago
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

10 years ago
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

10 years ago
(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

10 years ago
(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

10 years ago
Created attachment 256889 [details] [diff] [review]
rough idea for XUL
(Reporter)

Comment 7

10 years ago
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.
(Reporter)

Comment 8

10 years ago
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.
(Reporter)

Updated

10 years ago
Blocks: 376307
(Reporter)

Updated

10 years ago
No longer blocks: 353738
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INCOMPLETE

Comment 9

7 years ago
Mike, why did you marked this bug as incomplete?
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?
I did err, though once a regression is 3 years old one wonders if it's really still considered a regression!  Apologies nonetheless.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
RIP xforms
Status: REOPENED → RESOLVED
Last Resolved: 7 years agoa year ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.