Closed
Bug 77073
Opened 23 years ago
Closed 22 years ago
wallet checks slow context menu creation on forms containing select-one elements
Categories
(Toolkit :: Form Manager, defect)
Toolkit
Form Manager
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: bbaetz, Unassigned)
References
()
Details
(Keywords: perf, Whiteboard: [nav+perf] [Need ETA] [PDT-])
Attachments
(4 files, 5 obsolete files)
168.93 KB,
text/html
|
Details | |
160.65 KB,
text/html
|
Details | |
30.12 KB,
text/html
|
Details | |
13.49 KB,
patch
|
samir_bugzilla
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
Comment 2•23 years ago
|
||
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).
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•23 years ago
|
||
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)
Reporter | ||
Comment 4•23 years ago
|
||
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.
OS: Linux → All
Hardware: PC → All
Reporter | ||
Comment 5•23 years ago
|
||
> 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•23 years ago
|
||
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.)
nav triage team: Unfortunately, I don't think this performance improvement could be done without some more work in forms code. marking nsbeta1-
Keywords: nsbeta1-
Target Milestone: --- → Future
Comment 9•23 years ago
|
||
looking into nav+ performance, --> me
Assignee: morse → blake
Status: ASSIGNED → NEW
Whiteboard: [nav+perf]
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla0.9.3
Comment 10•23 years ago
|
||
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•23 years ago
|
||
Comment 12•23 years ago
|
||
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•23 years ago
|
||
I'm around. I'll look at it right now.
Comment 14•23 years ago
|
||
Comment 15•23 years ago
|
||
r=morse if you get rid of the dump('fsefesfsf')
Comment 16•23 years ago
|
||
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•23 years ago
|
||
checked in already.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 18•23 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 19•23 years ago
|
||
Comment 20•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Comment 25•23 years ago
|
||
what are the chances this will make the 094 branch?
Whiteboard: [nav+perf] → [nav+perf] [Need ETA]
Comment 26•23 years ago
|
||
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.....
Keywords: mozilla0.9.6
Target Milestone: mozilla0.9.3 → ---
Comment 27•23 years ago
|
||
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•23 years ago
|
||
We also spend about 15% of our time in js_GetProperty
Comment 29•23 years ago
|
||
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•23 years ago
|
||
js_GetProperty devolves (mostly) into nsDOMClassInfo::WrapNative and from there into XPCWrappedNative::GetNewOrUsed and XPCWrappedNativeScope::FindInJSObjectScope. Also nsCOMPtr_base::assign_from_helper
Comment 31•23 years ago
|
||
Comment 32•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
its a little to late for this one = PDT-
Whiteboard: [nav+perf] [Need ETA] → [nav+perf] [Need ETA] [PDT-]
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
Updated•23 years ago
|
Updated•23 years ago
|
Target Milestone: mozilla1.0 → mozilla0.9.7
Comment 36•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Attachment #31754 -
Attachment is obsolete: true
Attachment #39287 -
Attachment is obsolete: true
Attachment #39687 -
Attachment is obsolete: true
Comment 39•23 years ago
|
||
cc'ing sgehani and alecf for reviews
Comment 40•23 years ago
|
||
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?
Attachment #59673 -
Flags: needs-work+
Comment 41•23 years ago
|
||
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•23 years ago
|
||
Attachment #59673 -
Attachment is obsolete: true
Comment 43•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Attachment #59706 -
Attachment is obsolete: true
Comment 48•23 years ago
|
||
Comment on attachment 59716 [details] [diff] [review] Added comment that alecf requested thanks.
Attachment #59716 -
Flags: superreview+
Comment 49•23 years ago
|
||
Comment on attachment 59716 [details] [diff] [review] Added comment that alecf requested r=sgehani
Attachment #59716 -
Flags: review+
Comment 50•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
To solve it we can check in the patch in bug 112908 just as soon as the reviewers approve it.
Updated•23 years ago
|
Target Milestone: mozilla0.9.7 → mozilla0.9.9
Comment 54•23 years ago
|
||
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•23 years ago
|
||
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.
Updated•23 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.0.1
This is not a characteristic of a browser we want to call 1.0. Plussing.
Keywords: mozilla1.0+
Updated•22 years ago
|
Keywords: mozilla0.9.6
Comment 57•22 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → WORKSFORME
Updated•16 years ago
|
Assignee: morse → nobody
Product: Core → Toolkit
QA Contact: tpreston → form.manager
Target Milestone: mozilla1.0.1 → ---
Version: Trunk → unspecified
You need to log in
before you can comment on or make changes to this bug.
Description
•