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)
Tracking
()
VERIFIED
FIXED
People
(Reporter: morse, Assigned: mjudge)
References
()
Details
(Keywords: perf, regression, Whiteboard: [nsbeta3-])
Attachments
(1 file)
(deleted),
text/html
|
Details |
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.
Reporter | ||
Updated•24 years ago
|
Summary: Text entry on form with lots of elements if very slow → Text entry on form with lots of elements is very slow
Reporter | ||
Updated•24 years ago
|
Comment 1•24 years ago
|
||
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.
Comment 2•24 years ago
|
||
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.
Comment 3•24 years ago
|
||
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.
Comment 4•24 years ago
|
||
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
Comment 7•24 years ago
|
||
I'd like PDT to reconsider the [nsbeta2-] on this. It's a pretty serious
regression.
Whiteboard: [nsbeta2-]
Comment 8•24 years ago
|
||
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.
Comment 9•24 years ago
|
||
btw, on win32 m17 nightly, the bugzilla query page is also slow to respond.
Makes this more important I think.
Comment 12•24 years ago
|
||
M16 has been out for a while now, these bugs target milestones need to be
updated.
Comment 13•24 years ago
|
||
moving over to m17, adjusted priority and severity
Severity: major → critical
Priority: P3 → P1
Target Milestone: M16 → M17
Assignee | ||
Comment 14•24 years ago
|
||
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
Comment 16•24 years ago
|
||
I will attach the quantify run.
Also a snipit of functions calls sorted by function+descendants percentage of
total time.
Comment 17•24 years ago
|
||
Comment 18•24 years ago
|
||
oops that quantify really should be for bug 42451. ignore first attachment.
Comment 19•24 years ago
|
||
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).
Reporter | ||
Comment 20•24 years ago
|
||
I just added the patch for 40721 to my private tree and tried it. It didn't
help.
Assignee | ||
Comment 21•24 years ago
|
||
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.
Comment 22•24 years ago
|
||
Adding dbaron to CC list.
Comment 23•24 years ago
|
||
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...
Comment 24•24 years ago
|
||
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?
Reporter | ||
Comment 26•24 years ago
|
||
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.
Comment 27•24 years ago
|
||
Moving from [nsbeta2+] to [nsbeta2-] per todays Composer QA Beta2 bug review.
Comment 28•24 years ago
|
||
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.
Assignee | ||
Comment 29•24 years ago
|
||
changing milestone
Comment 31•24 years ago
|
||
According to comments on bug 45562, it might be worth fixing that bug first to
see if it helps this.
Comment 32•24 years ago
|
||
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.
Reporter | ||
Comment 33•24 years ago
|
||
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
Comment 34•24 years ago
|
||
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-]
Reporter | ||
Comment 35•24 years ago
|
||
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.
Comment 36•24 years ago
|
||
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
Reporter | ||
Comment 37•24 years ago
|
||
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
Reporter | ||
Comment 38•24 years ago
|
||
New bug for sample 7 is 47556.
You need to log in
before you can comment on or make changes to this bug.
Description
•