Closed Bug 42471 Opened 24 years ago Closed 24 years ago

Text entry on form with lots of elements is very slow

Categories

(Core :: Layout, defect, P1)

All
Windows NT
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: morse, Assigned: mjudge)

References

()

Details

(Keywords: perf, regression, Whiteboard: [nsbeta3-])

Attachments

(1 file)

This is a companion bug to 42451. That bug report deals with the fact that loading of the above url suddenly became very slow. This one deals with the fact that filling in any fields in the above url has suddenly become very slow. There is a noticeable delay between the time a character is typed and the time the echo of the character appears on the form. This delay was introduced sometime between 5PM on June 8 and 5PM on June 10.
Summary: Text entry on form with lots of elements if very slow → Text entry on form with lots of elements is very slow
Keywords: nsbeta2, perf, regression
Why is this bug assigned to me? Between the 8th and 5pm on 10th I checked in no string changes whatsoever. I made some string changes on the 10th after 5:49pm, but none that could have effected general performance. The chunking change I made for Simon didn't happen until nearly 8pm on the 11th. This is something you would know if you had performed even a cursory examination the checkin log. The following URL leads to my checkins for the last month <http://bonsai.mozilla.org//cvsquery.cgi?treeid=default&branch=&branchtype=match& dir=&file=&filetype=match&who=scc%25mozilla.org&whotype=match&sortby=Who&hours=2& date=month&mindate=&maxdate=&cvsroot=%2Fcvsroot> It took me on the order of 4 mouse clicks to bring up that log starting at my name in tinderbox. I see no evidence at all to date in either this bug report or the previous one (bug #42451) that you have taken step one to investigate what is happening in your own module to slow you down, be the cause internal or external to that module. All I see is that you made a guess based on names you saw go by in tinderbox. I thought it was very hospitable of waterson to spend the three minutes breaking into the debugger as he mentioned on the other bug (still assigned to me by you as of this writing) to get the big picture of where your code is being penalized. I can understand why you might not do this for some bug only tertiarily related to your code ... I do not understand why, on what for your code and by your own admission is a fundamental test case, you didn't take this elementary step yourself. I thought it was very hospitable of sfraser to spend all morning providing general speed-ups of the string handling code, and repairs to the internal interfaces of wallet and single sign-on. I thought it was even more hospitable of him to rein in his cogent comments on code quality. I respectfully suggest that wallet requires a new owner. One who is willing to work with you to bring the quality of your code up to an acceptable level. One who is willing to investigate performance problems to the depth expected of a module owner. I personally am one of the biggest fans of most of the privacy functionality you have introduced, Steve. I'm glad we have you to push these issues forward. I have said this to others both in private and in public. I'm happy to see you running the product every morning, and filing bugs on new regressions and blemishes. However, while I highly value the direction of your individual contributions, I don't think you meet the technical bar for an owner in mozilla. You yourself lend further evidence to this with repeated declarations of an inability to move forward because you are overburdened. I hope we can find an owner with the technical merit to match your vision and drive. If we do, I think wallet will mature into one of the most vital modules in the code-base. I think you will be happier working with such person, either still as an individual contributor, or as that persons manager. I think you will benefit and grow as an engineer in such an arrangement; and I imagine you will welcome someone to shoulder the specific burdens of ownership.
I just spent some time talking to Steve on the phone, and I think I should say this: 1. A bug report is not the right forum for some of the issues I brought up in my previous comment 2. Steve and I are working together to resolve technical, personal, and perceptual differences between us 3. I encourage anyone who has personal issues with Steve to bring them up directly with him 4. I encourage anyone who has technical issues with Steve to bring them up directly with him. Steve and I are working to understand where my possibly mistaken perception of his code quality stems. To that end, he and I have agreed to discuss other peoples constructive comments on his coding, so please cc me. If you are too shy to speak directly to Steve (but please, don't be) you can send your constructive comments directly to me to raise with him on your (anonymous) behalf. This bug report _still_ isn't the right place to be saying these things, but I had to head off any mistaken impressions left by the earlier comment. Steve and I are both looking forward to the improved working relationship both of us will have with others when these problems are resolved.
I left out that some of the initial confusion I had when responding innappropriately to this bug was that I mistakenly thought Steve was specifically filing a bug on performance degradation with respect to form fill-in. In fact, this is one of Steve's regular bug reports that I happened to remark on favorably in the last paragraph of the unfortunate comment.
This bug should go to mjudge, who can talk to layout folks to try to get it resolved. It's an ender-lite thang.
Assignee: scc → mjudge
Fix component.
Component: Browser-General → Layout
Hardware: PC → All
nsbeta2-
Whiteboard: [nsbeta2-]
I'd like PDT to reconsider the [nsbeta2-] on this. It's a pretty serious regression.
Whiteboard: [nsbeta2-]
I second Chris's comment. While the wallet interview is a pretty extreme form (in number of fields) it *is* shipping with the product. Worse, the problem shows up on several real-world commerce sites. Go to http://www.freeshop.com/pg005486.htm and click on the "Order Now" button. You must have PSM installed. The resulting form has the delayed typing problem, plus a wild expansion and retraction for every keypress.
btw, on win32 m17 nightly, the bugzilla query page is also slow to respond. Makes this more important I think.
[nsbeta2+][6/30]
Whiteboard: [nsbeta2+][6/30]
setting to m16, where + bugs live
Target Milestone: --- → M16
M16 has been out for a while now, these bugs target milestones need to be updated.
moving over to m17, adjusted priority and severity
Severity: major → critical
Priority: P3 → P1
Target Milestone: M16 → M17
I did some profiling today. *This is all from tables. The extra example and the one in the url field both have ender-lites inside of a table. ender-lite is in the same document as the webpage that contains it now. that means that when it adds content (aka the letter "a" for example), it causes a reflow of the whole document as it should. *Our incremental reflow is terrible. It seems that when inside a table the table does nothing to find out if the targeted content frame actually changed size or anything. it simply reflows the whole table. So it is doing a WHOLE lot of work just to add 1 character to a scrolled view inside a table. *The work inside editor is miniscule. Compared to the time for the document to reflow then to repaint its about .01%. painting SHOULD be the #1 time suck on profiles by far, but in my tests with Quantify it seemed that the problem was 50-50 between reflowing and actually repainting. This is unacceptable. *I saw comments alluding to the fact that "someday" incremental reflow will be better. I would hope that that day is now. I will try to get ahold of some layout folks to take a good hard look at this. If this incremental reflow cant handle even the simplest of modifications, then I am affraid that I will have to work to turn ender-lite off. That is also unacceptable. I am open for input. if anyone wants I can post quantify tables and/or call stacks.
Status: NEW → ASSIGNED
update qa contact
QA Contact: doronr → petersen
I will attach the quantify run. Also a snipit of functions calls sorted by function+descendants percentage of total time.
oops that quantify really should be for bug 42451. ignore first attachment.
mjugde, this test case is much too large to deal with. If you suspect the problem is that tables are reflowing when they shouldn't due to changes in ender lite widgets, a simple test case should be possible. Tables have a lot of reflow optimizations, but there are cases in which a change in one cell can cause all of the columns to change size and hence reflow every cell in the table. If all of the columns are not changing size and every cell is being reflowed then this is a bug. A few weeks ago, I fixed a bug where tables were invalidating too much of the table when an ender lite widget was modified. I seem to recall that in the test case, the ender lite widget's frame never changed size and that during the incremental reflow, only the cell was reflowed so that the ender lite widget could reflow. Also, as a long shot possible cause, I also forgot to put back in the fix for bug 40721 (the patch is attached to the bug).
I just added the patch for 40721 to my private tree and tried it. It didn't help.
yes that patch wont help this particular problem. the problem may be in nsBlockFrame::Reflow. nsBlockFrame(line 1697) here is the comment // this code does a correct job of propogating incremental reflows (bug 25510) // and has the potential to be very efficient. we should be able to // terminate reflow after the incremental reflow if we can detect that // nothing significant has changed. key word there being "potential". this code was never done. I will poke around some to see if I can come up with any easy solution. certainly this does not mean the table code doesnt also have problems, but I would rather hit block frame first.
Adding dbaron to CC list.
Based on a *very* incomplete understanding of the code, my guess as to why this wasn't done already is the following: * the overflow area, i.e., the area occupied by a block and all its children, is not stored by the block (it is passed through in the reflow metrics during reflow) * blocks don't care about overflow area. However, table cells and XUL boxes (and perhaps other things?) do. * if we stopped the incremental reflow based on the block's bounding box (borde edge) and margins (how do we handle margins here? that could be hard too, although it might be that you can just reflow the following block if the block itself is the target of the reflow) not changing, that would mess up incremental reflow for blocks contained in table cells There's a frame state bit called NS_FRAME_OUTSIDE_CHILDREN. I think it would work if we only did what we do now if we saw that NS_FRAME_OUTSIDE_CHILDREN was set either before or after the incremental reflow. However, all of this is based on almost no knowledge of how incremental reflow works. It's also based on my guess as to why it wasn't done already. I might be forgetting some other edge cases. I'll try and look into this...
Ignore my previous comments -- I was looking at the code earlier in nsBlockFrame::Reflow. I tried going through some simple cases in the debugger and seeing if nsBlockFrame::Reflow was called too much, and I don't see a case where I can see that there are more reflows than there should be. My understanding of how incremental reflow is even worse than I thought it was, so I don't think I can be of much help. One question: is typing performance worse in the text widgets than it is in the editor on a page of the same size?
added fix by date in status
Whiteboard: [nsbeta2+][6/30] → [nsbeta2+][8/2]
Now I'm confused as to what the date on the status line means. On the one hand, we had the pdt team putting dates there indicating that the bug would lose its pdt+ status on that date. On the other hand we had developers putting dates there indicating when they expected to have the fix done by. These are two different things and now I'm not sure which meaning was intended when beppe set the date to 8/2. Could we get a clarification on this -- not just for this bug but in general.
Whiteboard: [nsbeta2+][8/2] → [nsbeta2-][8/2]
Moving from [nsbeta2+] to [nsbeta2-] per todays Composer QA Beta2 bug review.
Michael, here is some data that Marc and I came up with. (1) A test with only 100 <input type=text>s each on a different line doesn't perform too badly when you rapidly type a bunch of different characters into one of the fields. (2) The same test increased to 150 shows noticeable degradation, increased to 300 shows much worse degradation. (3) A test with 150 radio boxes and 1 <input type=text> shows no degradation. (4) A test with 150 <input type=text>s all on the same line shows degradation but not quite as much as when they are on different lines (probably due to the unnecessary reflows happening on empty text frames.
changing milestone
Keywords: nsbeta2nsbeta3
Whiteboard: [nsbeta2-][8/2]
Target Milestone: M17 → M18
moving this to m19
Target Milestone: M18 → M19
Keywords: nsbeta3
According to comments on bug 45562, it might be worth fixing that bug first to see if it helps this.
I just filed bug 47395. Profiling typing on Win32 shows that 20% of the time is being spent grovelling through the <keyset>. We should read the DOM tree once, and build a hashtable.
If I understand the procedure correctly, anybody can nominate the bug for beta3 by putting nsbeta3 on the keyword line. If the group involved doesn't want to fix it for beta3, they then mark nsbeta3- on the status line. But the nomination of beta3 should still remain on the keyword line. So I'm renominating this for beta3 on the keyword. Furthermore, I want to make a strong appeal that it does get an nsbeta3+ rating. This is a serious performance hit. It is a regression that occured at a known time and it should have been fixed at that time when it would have been relatively easy to determine the offending check-in. In any case, it makes the product considerably slower than both nav4 and beta1 so I don't see how we can keep overlooking this.
Keywords: nsbeta3
this will be addressed with other perf issues after beta3, knowing that we are shipping with this 100+ form element page is an issue, but this is not the norm and is really more of an edge case.
Whiteboard: [nsbeta3-]
This is not an edge case. Go to the wallet sample pages accessible from the menu (tasks->privacy->form-manager->demo). There are ten sample forms which represent a random selection from the web. On two of forms (sample 7 and sample 10), the problem is evident (I can easily get way ahead of the echo characters by typing in a simple test sentence). It's true that the wallet interview form is by far the best example of the problem in that the delay of the echo characters is the greatest. Have we really decided to push the performance issues to post beta3? During beta2 we were told that performance issues were going to be addressed in beta3. This is the first that I've heard that there has been a change here.
on win98 using samples 7 & 10 -- text entry is fine, the delay that you mention is not visible, using bugzilla -- in this textarea, I don't see the issue either. It may have something to do with your set up, granted there are issues with speed on linux and on mac, but those issues are across application
On NT, the delay is significantly noticable on sample 7, much less so on 10. But now for some good news. Dave Hyatt just checked in a fix for bug 42451. And as a result of that fix I am seeing very little delay when typing on the wallet interview form. There still is a considerable delay on sample 7. However that has nothing to do with the number of elements (there are not too many of them) so it does not fall under the summary of this bug report. I'll open a separate report on sample 7 and close this one out as fixed.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
New bug for sample 7 is 47556.
Marking verified per last comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: