30091 - nsFormControlList::NamedItem() does a linear walk through the control list

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[rods (gone)]

When an option is added to a select via script a reflow is appended when the 
frame gets created. This is fine for adding one option, but if the page (like 
Bugzilla) has script that adds 100s of options, then hundreds of reflows are 
appended and dispatched. This causes horible performance and visual clutter.

To see this go Bugzilla and select a different component.

Comment 1

[Jesse Ruderman]

Is this a (special) case of Bug #17325?

Comment 2

[rods (gone)]

*** Bug 31278 has been marked as a duplicate of this bug. ***

Comment 3

[rods (gone)]

Yes, it is a special case of 17325

Comment 4

[rods (gone)]

*** Bug 31777 has been marked as a duplicate of this bug. ***

Comment 5

[rods (gone)]

*** Bug 31850 has been marked as a duplicate of this bug. ***

Comment 6

[rods (gone)]

*** Bug 33369 has been marked as a duplicate of this bug. ***

Comment 7

[Nisheeth Ranjan]

Accepting bug and setting target milestone to M16...

Comment 8

[Nisheeth Ranjan]

You are right that a lot of reflow commands are processed when we select an item 
in the "Program" list box on the bugzilla query page.  Once an item is 
selected, a javascript event handler adds and removes options from the version, 
target milestone, and component list boxes.  The event handler also 
intermittently gets and sets the selected property on the options inside the 
list boxes.  Each get or set call happens right after a couple of reflow 
commands are created and causes the commands to get processed immediately.  

When I changed the code in the option element such that GetSelected() and 
SetSelected() did not flush reflow commands, the number of reflow commands 
processed reduced drastically, because the first couple of reflow commands 
"absorbed" or coalesced the subsequent reflow commands within them.

Specifically, the number of reflow commands reduced from 38 to 6 for the case of 
clicking on the "brower localization" item once the "browser" item was selected.

Unfortunately, the performance problem in the "select an item in the Program 
list box" operation remained even when only 6 reflow commands were processed -- 
it took 3-4 seconds for the list boxes to get updated.  The only change in the 
"6 reflow command" case was that there was no visual flicker and jumpiness among 
the list boxes -- the screen updated all at once after 3-4 seconds.

My intuition says that we spend a lot of time in the frame constuctor as options 
are added and removed.  I'll use Quantify to test my hypothesis and report back 
here.

Comment 9

[rods (gone)]

*** Bug 31618 has been marked as a duplicate of this bug. ***

Comment 10

[Nisheeth Ranjan]

The quantify run yesterday pointed out nsFormControlList::NamedItem() as the 
performance hotspot.  It used to do a linear walk through the form control list 
and string comparisons to find the form control with a particular name.  We were 
spending a full 3 seconds in this method after we clicked on the "Browser 
Localization" item in the Program list box.

I have code in my tree that uses a hash table to perform these name lookups of 
form controls.  With these changes, the time taken by the "click on Browser 
Localization" item reduces from 5 seconds to 2 seconds.

Comment 11

[rods (gone)]

*** Bug 36021 has been marked as a duplicate of this bug. ***

Comment 12

[Nisheeth Ranjan]

The fix for this is checked in.  Updated summary to reflect what the real 
problem was.

I did a quantify run after making the hashtable changes and could not identify 
any low hanging fruit.  I'll show the quantify output to Troy tomorrow and see 
if he sees anything special.

Comment 13

[R.K.Aa.]

Visual clutter: "take two".
On bugzilla query page: If i now select "Browser" as component under "Program"
the boxes to the right move around a lot horisontally, growing and shrinking and
painting garble on screen till they 15 seconds later (!!) have decided how many
and where they are. In the meanwhile something called gShellCounter counts from
504 to 824 in console, for instance. Attaching screenshot, 19k.
Tested on a P120 with build ID 2000-041908-M16 Linux.

In comparition: With NC4.7 same operation redraws instantly on same PC, i only
see the "component" box shake for a second as it estimates it's textual content.
The dimensions and placement of the box is already known.

Comment 14

[R.K.Aa.]

Attachment: 19k garbled layout (two samples)

Comment 15

[R.K.Aa.]

More recent garble in bug 36403

Comment 16

[rods (gone)]

I was wondering if you considered a timer so it wouldn't actually process any of 
the reflows until the timer went off, that way as new reflows were appended the 
timer could be stopped and reset. That way maybe only one reflow wouild take 
place.

Comment 17

[Nisheeth Ranjan]

Reopening bug...

Rod, we coalesce reflow commands already, but, the problem in this case is that 
we flush them each time GetSelected() is called on the option frame.  We need to 
figure out a way to avoid the flushing.  I have some thoughts on that and I will 
call you to discuss them today.

Comment 18

[rods (gone)]

*** Bug 36259 has been marked as a duplicate of this bug. ***

Comment 19

[Sean Richardson]

The remainder of this bug, after fixing nsFormControlList::NamedItem() to use a 
hash, seems to be identical to bug 18189, "[INC CONTENT SINK] select scrollbars 
display multiple times on js enhanced form".

Comment 20

[Nisheeth Ranjan]

I agree.  I'm going to mark 18189 a dup of this one.

Comment 21

[Nisheeth Ranjan]

*** Bug 18189 has been marked as a duplicate of this bug. ***

Comment 22

[R.K.Aa.]

Does the reflow refresh the titlebar too in preferences?
Tested linux build 2000-042416 M16
When opening preferences/fonts the titlebar actually "animated", allthough
jagged, due to frequent refreshes originating from "somewhere". Went on for a
while. 
This page in prefs - different from most - have dropdown boxes for selections
(There's regression on linux right now making these refreshes appearant, bug
20496.)

If you test this: use the build i mentioned, cause the whole font-pref has
vanished in later builds.

Comment 23

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Nominating nsbeta3.  This causes very bad visual behavior and performance on the
bugzilla query page, and probably other pages.

Comment 25

[Sean Richardson]

I concur. From the DUP bug 18189, some eBay pages behave atrociously viewed 
with Mozilla. Adding top100 keyword.

Comment 26

[Peter Trudelle]

nsbeta3+, M18, lower priority.  reducing severity to normal (its now just 
cosmetic)

Comment 27

[Nisheeth Ranjan]

Marking nsbeta3+

Comment 28

[Nisheeth Ranjan]

This bug has been marked nsbeta3- because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration, but do not clear 
the nsbeta3- nomination.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Nominating for dogfood.  I don't change the Product field in the Bugzilla Query
page in Mozilla.  If I want to do a search that uses that field, I start up 4.x,
since 4.x starts faster than Mozilla takes to just click on an item in the
Product field.  (And if I want to select 3 items...)

Comment 30

[leger]

Putting on [dogfood-] radar.  David, plase go see nisheeth directly on this.

Comment 31

[buster]

I agree with DBaron on this one.  Sure seems like dogfood to me. Is this one we 
could hand off to rickg?  He's chomping at the bit for perf problems.

Comment 32

[R.K.Aa.]

*** Bug 63090 has been marked as a duplicate of this bug. ***

Comment 33

[Gervase Markham [:gerv]]

Removing mostfreq, as this is already covered by bug 27233.

Gerv

Comment 34

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Nisheeth here, typing on Heikki's computer.  This bug is no longer at the
dogfood level of criticality.  So, removing the dogfood keyword so that this bug
does not show up in the weekly "Key Bug Metrics" report run by chofmann.

Comment 35

[Nisheeth Ranjan]

The more general problem of filling in selects from JS is covered by bug 27233.  
This bug itself is fixed because we no longer do a linear walk through the form 
control list.

Marking bug fixed.

Comment 36

[Chris Petersen]

Marking verified per last comments.

Comment 37

[Chris Petersen]

Marking verified

Comment 38

[Chris Petersen]

Verified in the May 30th build.