77073 - wallet checks slow context menu creation on forms containing select-one elements

Status: RESOLVED WORKSFORME

(Toolkit :: Form Manager, defect)

Description

[Bradley Baetz]

This comes out of bug 77051 - see additional comments by me there.

If you are a /. moderator, and visit
http://slashdot.org/article.pl?sid=01/04/21/0148202&mode=nested
then bringing up a context menu takes about 4.5 to 5 seconds on linux. The above
kuro5hin url shows the same effect, but only takes a couple of seconds to bring
up the context menu. On the /. url, there were 194 elements in the framesArray
in getState.

Profiling data is available in
http://bugzilla.mozilla.org/show_bug.cgi?id=77051. If you look at the
hierarchical data (and my comment in bug 77051 at 2001-04-21 17:08), it turns
out that the root of the time is nsWalletlibService::WALLET_PrefillOneElement
(62% of the time that function is in the stack)

getStateFromFormsArray (in walletOverlay.js) has a threshold (which is 0 for the
context menu), but the counter isn't incremented for select-one elements, and
WALLET_PrefillOneElement is called anyway for these elements. I'm going to
attach a patch which brings this down to the 0.5 seconds level on linux. I do
not claim that it is correct, and in fact it probably isn't.

However, it appears from the profiling that WALLET_POE spends most of its times
in string functions. It also appears to do initialisation each time its called.

The context menu ends up calling this all twice, once for each of the two items,
which doesn't help matters either.

Comment 1

[Bradley Baetz]

Attachment: hack

Comment 2

[Stephen P. Morse]

The attachment may speed things up, but it is wrong.

What wallet is doing is determining if any of the fields can be prefilled and, 
if so, enables the prefill item in the context menu.  What the patch is doing is 
skipping all the drop-down lists and testing only the text fields.  So if there 
is a drop-down list that could be prefilled but there aren't any textfields that 
can, you will wind up disabling the prefill menu item which is not correct.

If you really want to speed things up, then we could bypass this test altogether 
and always enable the menu item.  However then the UE people will complain 
because they have said that if there is nothing to be prefilled it is confusing 
to the users to have the menu item enabled.

I guess the correct thing to do here is figure out why the string comparisons 
for PrefillOneElement are slower on linux than they are on win32 (from what's 
been said, we don't have this slowdown on win32).

Comment 3

[Bradley Baetz]

Its definately wrong, but the patch does show that the performance problem is
present.

Why is:

if (type != "select-one") {
  elementCount++;
}

present? Removing the if test would probably have the same effect.

I have no idea why strings are so slow - I'm using the 2-byte wchar_t, so
conversions shouldn't be a problem. What I'm seeing doesn't mean that they are
slow though - they could just be being called lots of times.

Ben Goodger sees this on a windows debug build but not an optimised one
(although he said that it is very slightly slower than usual)

I see this slowdown on both a linux debug -O2 and a linux nightly build
(although obviously the slowdown is less with the a --disable-debug build - the
slashdot page when I am a moderator takes 5 seconds instead of 7)

Comment 4

[Bradley Baetz]

And while the above url is only about 0.5-1 seconds slower, a nightly windows
build still takes 3 seconds to load up the slashdot page. (That 0.5 seconds may
be why Ben didn't notice it)

So this isn't a linux-only issue. I'll see if I can make up a simple test case
tomorrow.

Comment 5

[Bradley Baetz]

> And while the above url is only about 0.5-1 seconds slower, a nightly windows
> build still takes 3 seconds to load up the context menu on the slashdot page.
                                         ^^^^^^^^^^^^^^^^^^^

Oops, sorry.

Comment 6

[Jesse Ruderman]

See also bug 54300, autofill items shouldn't be in document context menu.  
(They might be retained for form context menus, though, so speeding up this 
check wouldn't hurt.)

Comment 7

[Paul Chen]

nav triage team:

Unfortunately, I don't think this performance improvement could be done without 
some more work in forms code. marking nsbeta1-

Comment 8

[Blake Ross]

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

Comment 9

[Blake Ross]

looking into nav+ performance, --> me

Comment 10

[Blake Ross]

I looked into this a bit today and noticed a few things: we retrieve the 
wallet.crypto pref for each form element on the page, and get the wallet service 
twice, each time the context menu is opened. Fixing that had a somewhat 
noticeable impact on the time it took to bring up the context menu, in my debug 
build.

Still, we're doing a heck of a lot of work just to disable/enable/hide/show 
these items.  I don't think it's worth it.  I think we should just remove these 
items from the menu, for now.  Cc'ing Matthew for possible alternatives (morse 
is already cc'd).

Comment 11

[Blake Ross]

Attachment: patch for the interim

Comment 12

[Blake Ross]

Steve, if you're around, could you r= this patch until we decide what to do?  
I'm hoping to get it into .9.2 if it's the right thing to do.

Comment 13

[Stephen P. Morse]

I'm around.  I'll look at it right now.

Comment 14

[Blake Ross]

Attachment: updated patch

Comment 15

[Stephen P. Morse]

r=morse if you get rid of the dump('fsefesfsf')

Comment 16

[Matthew T (active 1999-2002)]

Assuming that the user is only going to submit one form on any given instance 
of a given page, it won't hurt to auto-fill all forms on a page at once. 
Therefore, the context menu items could be replaced by a `Fill in Forms' item 
at the top level of the `Tasks' menu.

Comment 17

[Blake Ross]

checked in already.

Comment 18

[Ashley Bischoff (blog at handcoding.com)]

Reopening bug, as this still occurs for me (please correct me if the
slow-context-menu-with-large-forms bug that I encountered is different from this
one).

See below attachment for testcase.

Comment 19

[Ashley Bischoff (blog at handcoding.com)]

Attachment: Testcase: Save-As from Slashdot (with moderation privileges).

Comment 20

[Ashley Bischoff (blog at handcoding.com)]

On the testcase (attachment 49507 [details]), the only changes that I made to the source
code was to change the ACTION on all FORM tags to "?", to deactivate the forms
(I wouldn't want Mozilla people to be moderating Slashdot as me, heh).

The bug isn't affected by those changes.

Comment 21

[Mike Pinkerton (not reading bugmail)]

this still happens on 10/2 branch bits, macosx. i profiled it and it's wallet
for both Context menus and the Edit menu.

Comment 22

[Paul Chen]

A nice test case is www.fileflash.com. On my 500MHz PowerBook G3, it takes about
5 secs for the edit menu to show up. On my dell dimension 210 with a P3500
runnign RedHat 7.1, it takes about 6 secs for either the edit menu or context
menu to show up.

Comment 23

[timeless]

rjesup has jprof data which he will attach today. I'm going to file a few bugs 
w/ some patches based on the things we found.  Most of these are outside wallet 
so the eventual direction of wallet won't matter.

Basically the bottom line is that wallet does its own dom walking which is very 
expensive [::IndexOf, ::Get*Sibling*]. it's kind of similar to the problems 
find had until recently).  There's also some QI action which doesn't help 
wallet, so if we bloated the code w/ an alternative method that skipped QIs we 
might save sometime.

I think an alternative would be to use hyatt's suggestion to the link toolbar 
people of using xbl (or c++) to generate events [in this case against <input>, 
in links case it was against <link>] which chrome (wallet) would catch each 
element and then do whatever processing it needs.  My only concern is that it 
would be hard to determine adjacency of input widgets and some other magic that 
wallet has been doing.  Hyatt: does this sound reasonable?

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Attachment: jprof of clicking on the Edit menu (also the File menu, but that came up instantly). Linux, dual PIII/450's, -O2, recent pull

Comment 25

[Jaime Rodriguez, Jr.]

what are the chances this will make the 094 branch?

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

The relevant portion is
http://bugzilla.mozilla.org/attachment.cgi?id=52225&action=view#95701.

Also NodeName:
http://bugzilla.mozilla.org/attachment.cgi?id=52225&action=view#45552

and AtomImpl::ToString
http://bugzilla.mozilla.org/attachment.cgi?id=52225&action=view#3415

Minor optimizations for those (etc) will be filed as separate bugs (and put as
dependencies here).  This one (rewrite the DOM walking code or use other DOM
walking code) is more work.  Some of the work in Find could be leveraged, of
course.  

Are you sure you don't mean 0.9.5?

In any case, it's not going to make 0.9.3.....

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

wallet_ResolveStateSchema calls text.Find O(N*M) times, where N = number of DOM
nodes (with text?), and M = number of schema nodes.  Because of this, Find shows
up as around 9% of total time to open the menu, with another 2% direct hits in
wallet_ResolveStateSchema itself (and of course wallet_StepForwardOrBack as
previously mentioned accounts for ~60%).

We also spend a lot of time (partly/mostly within the above routines) in
creators/destructors for nsAutoString (together around 10%).

As for QI, we spend about 20% of our time in QI; mostly in
GetPrevious/NextSibling and GetLastChild.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

We also spend about 15% of our time in js_GetProperty

Comment 29

[Brendan Eich [:brendan]]

rjesup: you mean "under", not "in" (I hope!).  What fan-out from js_GetProperty
dominates?  It is likely to be a DOM getter (or several).

/be

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

js_GetProperty devolves (mostly) into nsDOMClassInfo::WrapNative and from there
into XPCWrappedNative::GetNewOrUsed and
XPCWrappedNativeScope::FindInJSObjectScope.  Also 
nsCOMPtr_base::assign_from_helper

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Attachment: jprof with "-i js_GetProperty" (from a new run of just opening/closing Edit menu)

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

I did another jprof (can be supplied if needed) of only the Edit menu (previous
one also included the file menu), and used it for the -i jprof above.  The full
jprof is available, but it's substantially the same as
http://bugzilla.mozilla.org/attachment.cgi?id=52225&action=view, however the %'s
for the problem routines are even higher: 72% in StepForwardOrBack, 27% in
nsGenericHTMLElement::QueryInterface, 19% in XPCWrappedNative::GetNewOrUsed, 12%
in GetNodeName, etc.

Comment 33

[Bradley Baetz (:bbaetz)]

My comment from 2001-04-21 22:08 has still not been answered. Can anyone tell me
why wallet does stuff that way?

Comment 34

[Jaime Rodriguez, Jr.]

its a little to late for this one = PDT-

Comment 35

[Blake Ross]

--> morse

Comment 36

[Stephen P. Morse]

For an extreme example of this on all platforms, bring up the following URL and 
then open the edit menu or the context menu.  On my 450 MHz NT machine, the edit 
menu takes 5 seconds and the context menu nearly 6 seconds.

   file:///y:/mozilla/extensions/wallet/editor/interview.html

(note: modify the path above depending on where your mozilla source tree it)

Comment 37

[Stephen P. Morse]

Currently there are two separate passes -- one to determine if the 
wallet-capture item should be enabled and the other to determine if the 
wallet-prefill item should be enabled.  And these passes are expensive.

The first obvious thing to do is determine the state of both items in a single 
pass.  That will cut down the time required by a factor of 2.  Attaching a patch 
that accomplishes this.

It's still not enough but at least it's a substantial start.  Next step will be 
to optimize the processing done in the pass.

Comment 38

[Stephen P. Morse]

Attachment: Combine the two passes into one.

Comment 39

[Stephen P. Morse]

cc'ing sgehani and alecf for reviews

Comment 40

[Alec Flett]

Comment on attachment 59673 [details] [diff] [review]
Combine the two passes into one.

ok, I _mostly_ get what's going on...the code looks good, but I have a few
questions:

where are 'hide' and 'enable' defined? i.e. they're used kind of like
constants, but I don't see them defined as constants:
+ const bothHide = {capture: hide, prefill: hide};

here, can you comment about why you're changing all hides to disables:

+	     for (var j in bestState) {
+	       if (bestState[j] == hide) {
+		 bestState[j] = disable;
+	       }

and finally, can you explicitly document all the possible 'states' - i.e. what,
enable, disable, hide, true, and false each mean?

Comment 41

[Stephen P. Morse]

Attaching new patch to address alecf's comments.  Here are the answers to his 
questions:

1. hide, disable, and enable were defined in the middle of the file.  Those 
definitions were not changed which is why they didn't appear in the diff.  
However they were defined as a "var" instead of a "const" so I just corrected 
that.  Also I just moved the definition to the head of the file, along with the 
other consts so they can be easily spotted

2. To understand why I am changing all hides to disables, you have to see that 
code in its full context (it's hard to understand the logic from a diff).  The 
code appears in a section in which I discovered that the form has at least one 
text or select element (there is a comment preceding this code that states so).  
In that case, I want the capture and prefill menu item to be visible (i.e., not 
hidden).  That means that if it is already enabled, then I want to leave it 
alone but if it is hidden I want to upgrade it to disabled (visible but grayed 
out).

3. I just added comments defining the use of enable, disable, hide, true, and 
false

Comment 42

[Stephen P. Morse]

Attachment: combine the two passes into one (with alecf's comments addressed)

Comment 43

[Alec Flett]

Comment on attachment 59706 [details] [diff] [review]
combine the two passes into one (with alecf's comments addressed)

excellent. thanks for the comments.

Actually, I'm still confused looking at the file itself, and the patch doesn't
necessarily improve the situation. I'll go by the patch's updated state:

you have this line:
+      var bestState = bothHide;

and then the very next reference to bestState is:
+	     for (var j in bestState) {
+	       if (bestState[j] == hide) {
+		 bestState[j] = disable;
+	       }
	     }


none of the code in between these two lines changes bestState, so seems like
the best option here is just to make the first line:
+ var bestState = bothDisable;

and remove the second chunk I just quoted.
(You'll have to define bothDisable of course)

Comment 44

[Stephen P. Morse]

But in between those two lines is the statement:

  if ((type=="") || (type=="text") || (type=="select-one")) {

So the initial value of "bothHide" is changed if and only if we encounter a text 
element or a select element.  If none is encountered, then the initial setting 
remains.  So you suggestion of making the initial value be bothDisable would be 
incorrect.

Comment 45

[Alec Flett]

Comment on attachment 59706 [details] [diff] [review]
combine the two passes into one (with alecf's comments addressed)

ah, ok it's still not clear why we do this then.

can you just add a comment explaining this then? that's all I asked for
originally. comments are cheap.

Comment 46

[Stephen P. Morse]

Let me try to anticipate your next question.  You are going to say that since 
there was no code that changed bestState in between the two lines you cited, 
then the value of bestState has to be bothHide so why doesn't the code simply 
change it to bothDisable without doing any testing.  That's because there is 
code following this section of code that can set bestState to other values.  
Note that in between the two lines you've cited there is a "for" statement so we 
could have gone around a loop several times and the value of bestState is no 
longer bothHide at the time that the test-and-assignment is being done.

Comment 47

[Stephen P. Morse]

Attachment: Added comment that alecf requested

Comment 48

[Alec Flett]

Comment on attachment 59716 [details] [diff] [review]
Added comment that alecf requested

thanks.

Comment 49

[Samir Gehani]

Comment on attachment 59716 [details] [diff] [review]
Added comment that alecf requested

r=sgehani

Comment 50

[Stephen P. Morse]

Patch to combine the two passes into one has been checked in.  Still want to do 
more optimizing here, but a time reduction of 50% was certainly a good start.

Comment 51

[Jeremy M. Dolan]

Nice fix... unfortunatly this is just one more reason why the wallet UI needs to
die, die, die.

Note the new context menus do not have any wallet entries, which is a good
start: http://mozilla.org/projects/ui/menus/shortcut/

Comment 52

[Neil Pryde]

Ok, this is new: http://www.mozdev.org/bugs/show_bug.cgi?id=697

What can we do to solve this issue?

Thanks,
-Neil.

Comment 53

[Stephen P. Morse]

To solve it we can check in the patch in bug 112908 just as soon as the 
reviewers approve it.

Comment 54

[Stephen P. Morse]

See also bug 64357.  Turns out that the tests for enabling/disabling the menu 
items are not working correctly.  That's of course related to this bug.

Comment 55

[Samir Gehani]

Steve,
One idea you may consider is leaving the items available and when clicked then
compute if the form can be prefilled or data can be captured.  If there is no
form to prefill or data to capture toss up a dialog indicating there must be a
form to prefill or that there must be data to capture.  We avoid the context
menu performance hit while trading off with IMHO a small usability hit.  Just a
thought for consideration.

Comment 56

[Mike Shaver (:shaver emeritus)]

This is not a characteristic of a browser we want to call 1.0.  Plussing.

Comment 57

[Stephen P. Morse]

No longer an issue now that wallet items are not on the context menu.  However 
it still is an issue for the edit menu.  Bug 130424 which was just opened, 
addresses this issue in the edit menu.

Closing this one as WFM since it is no longer an issue.