473178 - Firefox is very slow to create and initialize a lot of input tags. [tracking]

Status: RESOLVED WORKSFORME

(Core :: Layout: Form Controls, defect)

Description

[Florian]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.0; SLCC1; .NET CLR 2.0.50727; Media Center PC 5.0; .NET CLR 3.0.04506; InfoPath.2; .NET CLR 1.1.4322)
Build Identifier: 3.0.5

I click on create to see the table. After I click on "init table" to initialize and clear the table.
Firefox does this in 1200 milliseconds.
IE 7 does this in 275 milliseconds.
Opera and Safari do this this in 50 milliseconds.

Reproducible: Always

Steps to Reproduce:
1.Click on table
2.Click on init table
3.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090111 Minefield/3.2a1pre

I get a result of 4254 ms.. I tried also older builds but it was never fast in Firefox.

Comment 2

[Florian]

Did you try with IE, Opera or Safari ?
I tried Firefox 3.1 beta 2 and Firefox 3.2 alpha and I have the same problem.
It's too slow.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So... You're not actually timing creation there, are you?  You're just timing the init + clear calls as far as I can see.  I the "create" in the summary correct?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase, with Shark profiling hooks added when starting and stopping the timer

Comment 5

[Boris Zbarsky [:bzbarsky]]

So a profile of exactly what that testcase is timing shows that we spend about 70% of our time under nsPlaintextEditor::InsertText and another 17% under nsFrameSelection::SelectAll.  The rest is minor things like InitFocusedValue, GetText, GetValue, JS execution, etc.

Under SelectAll, it's basically SetSelected calls of various sorts.

Under InsertText, we basically have nsTextEditRules::WillInsertText (which does various stuff with WillInsert, deleting selection, InsertTextImpl, etc, etc), and EndPlaceholderTransaction, which spends 25% of total time under nsViewManager::EnableRefresh: 7% flushing pending invalidates and 17% doing the sync paint.

Bug 174823 covers the sync behavior there.

Bug 221820 covers us not wanting to use the editor at all unless forced to.

Comment 6

[Jean-Francois Ponce]

Same problem on my side, a simple 

document.getElementById('myinput').value='test';

take around 160ms on Vista / FF 3.5.5.

Other browser -> Chrome 4 - 7ms / Safari - 6ms / Opera 10 - 1ms

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment 6 seems to have nothing to do with this bug.  Please file a separate bug on it and attach your testcase.

Comment 8

[Jean-Francois Ponce]

Hi Boris,

If I'm right we are talking here about a slow creation / initialisation of input tags. My comment fit with the initialisation problem.

More details about my test case :

var elem=document.getElementById('myinput');
-> start_time
elem.value='test';
-> end_time

end_time-start_time = 160ms on FF 3.5.5 ( Other browser between 1 and 7ms )

Regards


(In reply to comment #7)
> Comment 6 seems to have nothing to do with this bug.  Please file a separate
> bug on it and attach your testcase.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Jean, this bug is about a specific testcase.  Please file a separate bug with an HTML file that actually shows your problem.  The information in comment 0 is clearly insufficient for that purpose (e.g. over here that operations is in the sub-ms range).

Just to be clear, "slow behavior X" is not a bug.  It's a symptom.  In most cases, different causes can lead to that same symptom, which is why there should be one bug per testcase.

Comment 10

[Paul A. Stølen]

Attachment: A simple benchmark of the time taken to change the .value property on an input field.

Adding another test case (since this seems to be required to have a voice in the discussion, and because nobody here is likely to bother with going through the steps necessary to reproduce it where it first appeared).

I've encountered the problem described in this bug description in an editor i made, it stalls for quite a while when generating and updating input fields for large data sets.

Note: The problem appears to get worse in large node trees. Setting the value of input elements in the context where i first encountered this problem, takes about 4 times as long as in this test case - on average.

Comment 11

[(no longer active)]

Comment on attachment 357388 [details]
The testcase, with Shark profiling hooks added when starting and stopping the timer

I tried to benchmark this with a build including the patch for bug 221820, and for one without it.  Without that patch, I averaged at 235ms, and with a build including that patch, I averaged 13ms.

Chromium trunk averages 4ms, and Safari 4.0.4 averages at 26ms.

I sharked the build with bug 221820, and we're spending 13.7% of our time in GetElementById, and 79.1% of it in nsHTMLInputElement::SetValue.  Here's a breakdown on what happens in SetValue:

69.3% in nsTextControlFrame::UpdateValueDisplay
  38.2% in nsGenericElement::RemoveChildAt, which is almost entirely spent in nsFrameManager::RemoveFrame.
  13.7% in nsTextControlFrame::SetPlaceholderClass, which is almost entirely spent in nsGenericElement::SetAttr.

Comment 12

[(no longer active)]

Comment on attachment 431920 [details]
A simple benchmark of the time taken to change the .value property on an input field.

Here are the results for this test case.

Without bug 221820:

Stats for Creating input element:
	Average:	0.22 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	199 milliseconds (97.1% of time elapsed during usage)

Stats for Setting input value:
	Average:	0.54 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	486 milliseconds (98% of time elapsed during usage)

With bug 221820:

Stats for Creating input element:
	Average:	0.1 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	89 milliseconds (98.9% of time elapsed during usage)

Stats for Setting input value:
	Average:	0.08 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	71 milliseconds (89.9% of time elapsed during usage)

Chromium trunk:

Stats for Creating input element:
	Average:	0.01 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	5 milliseconds (83.3% of time elapsed during usage)

Stats for Setting input value:
	Average:	0 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	4 milliseconds (100% of time elapsed during usage)

I also sharked the set value part of the test case.  We're spending 85.1% of the time in nsHTMLInputElement::SetValue, and the breakdown matches what I reported in the previous comment.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why does setting the value always create a new text node instead of reusing an existing node if available? That seems to be the big remaining problem here.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also I expect that lazy frame construction will help here --- at least in terms of the reported benchmark results --- since it will at least move the frame construction for text inputs outside the timing interval.

Comment 15

[Timothy Nikkel (:tnikkel)]

Unfortunately not, because I had to disable lazy construction of nodes which are editable. Perhaps we should get a follow up bug to enable lazy construction of editable nodes.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does that mean construction of the nsTextControlFrame for an <input> is not lazy?

Comment 17

[Timothy Nikkel (:tnikkel)]

Yes.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why is that? It's not really editable. The anonymous DIV inside it should be the root of the editable subtree.

Comment 19

[Timothy Nikkel (:tnikkel)]

To be specific, nsINode::IsEditable is what we check when attempting to construct lazyily. nsGenericHTMLFormElement::UpdateEditableFormControlState sets the NODE_IS_EDITABLE flag on inputs that are textboxes, which IsEditable checks. Lazy frame construction used IsEditable because it was convenient and solved problems with the editor expecting frames to be constructed synchronously. Maybe we can refine this check so that input textboxes can be lazy, which are used on a lot of pages.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, that'd be great. Please spin off a separate bug for that, and we can make this bug depend on that.

Comment 21

[(no longer active)]

(In reply to comment #13)
> Why does setting the value always create a new text node instead of reusing an
> existing node if available? That seems to be the big remaining problem here.

It doesn't; it will use an existing text node if there is one.  This test, however, sets the value on each text box only once, so it's a matter of how many times a new value is set, and therefore how many times a new text node needs to be created.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK.

Regarding comment #11, the SetPlaceholderClass stuff should go away when we fix bug 553097 I guess.

Why is nsGenericElement::RemoveChildAt being called? The editor should not be initialized and the value is empty, so shouldn't the anonymous DIV be empty before we set the new value?

Comment 23

[(no longer active)]

(In reply to comment #22)
> OK.
> 
> Regarding comment #11, the SetPlaceholderClass stuff should go away when we fix
> bug 553097 I guess.

Hopefully, yes.

> Why is nsGenericElement::RemoveChildAt being called? The editor should not be
> initialized and the value is empty, so shouldn't the anonymous DIV be empty
> before we set the new value?

A11y expects the native anonymous content of an input control to be the exact same whether or not an editor is initialized for the control, which means that when the value is empty, there should be a br node in the anon tree, and when it's not, there should be a test node.  See bug 221820 comment 95 for more information.

What happens in this case is that the input control is created with an empty value, which places a br element in the native anonymous tree, and then a value is set on the control, which means that the br node should be removed and a text node should be appended instead of it.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Perhaps we can do a simple hack to accessibility to handle the case where there's no <br> in the empty control?

Comment 25

[(no longer active)]

(In reply to comment #24)
> Perhaps we can do a simple hack to accessibility to handle the case where
> there's no <br> in the empty control?

When I was looking into this problem, a simple way to fix this problem wasn't evident.  I spent several days trying to debug it, and couldn't nail down what exactly is happening inside the a11y module which causes this.  This was partly because I wasn't familiar with the code, and partly because a11y basically goes ahead and touches the native anon tree, assuming that an editor is present.

I think the right thing to do here is for a11y not to use the native anon tree, but when I was discussing this with David, he said that doing that requires a lot of effort, that's why I worked around the problem this way.

Now that I think of it, if it's possible to detect at runtime whether the accessibility service is active or not, we can switch the logic based on this information, so that at least only those users who have a11y turned on pay the price here.  David, is that possible?

Comment 26

[Timothy Nikkel (:tnikkel)]

Would nsIPresShell::IsAccessibilityActive work for that?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you can turn on accessibility dynamically so that wouldn't really work.

Maybe you could post a patch which disabled <br> injection for empty inputs, and then David could make a patch to fix the accessibility code to handle that?

Comment 28

[Timothy Nikkel (:tnikkel)]

nsIPresShell::IsAccessibilityActive seems to have code to handle accessibility being turned on/off dynamically.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah but what if someone turns on accessibility, would we then do a frame reconstruct on every document or something?

Comment 30

[Timothy Nikkel (:tnikkel)]

Oh, good point.

Comment 31

[(no longer active)]

(In reply to comment #27)
> Maybe you could post a patch which disabled <br> injection for empty inputs,
> and then David could make a patch to fix the accessibility code to handle that.

Filed bug 559710 for that.

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #25)
> (In reply to comment #24)
> > Perhaps we can do a simple hack to accessibility to handle the case where
> > there's no <br> in the empty control?

You would think.

> 
> When I was looking into this problem, a simple way to fix this problem wasn't
> evident.  I spent several days trying to debug it, and couldn't nail down what
> exactly is happening inside the a11y module which causes this.  This was partly
> because I wasn't familiar with the code, and partly because a11y basically goes
> ahead and touches the native anon tree, assuming that an editor is present.
> 
> I think the right thing to do here is for a11y not to use the native anon tree,
> but when I was discussing this with David, he said that doing that requires a
> lot of effort, that's why I worked around the problem this way.

I'm not sure it is too much effort, though I do recall your debugging stories being exciting :)

> 
> Now that I think of it, if it's possible to detect at runtime whether the
> accessibility service is active or not, we can switch the logic based on this
> information, so that at least only those users who have a11y turned on pay the
> price here.  David, is that possible?

Yes, but there is at least one gotcha we know about (Roc's).

(In reply to comment #27)
> I think you can turn on accessibility dynamically so that wouldn't really work.
> 
> Maybe you could post a patch which disabled <br> injection for empty inputs,
> and then David could make a patch to fix the accessibility code to handle that?

I recall Ehsan having a patch for the accessibility code that special cased this, but I don't recall why it didn't land.

Alexander?

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)

> > Maybe you could post a patch which disabled <br> injection for empty inputs,
> > and then David could make a patch to fix the accessibility code to handle that?
> 
> I recall Ehsan having a patch for the accessibility code that special cased
> this, but I don't recall why it didn't land.

> Alexander?

I remember only one Ehsan patch for accessibility code (bug 542824) but if I get right it wasn't related with <br> stuffs

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm actually not convinced these test failures indicate an accessibility breakage in this case. (I'll elaborate in bug 559710)

Comment 35

[(no longer active)]

(In reply to comment #32)
> I recall Ehsan having a patch for the accessibility code that special cased
> this, but I don't recall why it didn't land.

Well, I didn't really have a patch, I spent 2-3 days delving into accessibility code trying to figure out this problem, but that effort failed, and I gave up eventually!

Comment 36

[Timothy Nikkel (:tnikkel)]

Filed bug 559970 for comment 20 (and previous).

Comment 37

[Boris Zbarsky [:bzbarsky]]

Got a few questions:

1)  If you time the whole 900-input operation, is the time you get consistent
    with the times the bench stuff reports?  Note that bench is adding a bunch of
    0s and 1s, basically, right?
2)  Does webkit (esp. the chromium version) do a sync version of
    UpdateValueDisplay on value set?  Or do they just mark it as needing to  
    happen?  Should we just mark it as needing to happen?

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to comment #37)
> 1)  If you time the whole 900-input operation, is the time you get consistent
>     with the times the bench stuff reports?  Note that bench is adding a bunch
> of
>     0s and 1s, basically, right?

I checked this, the numbers it gets are basically correct, the resolution of the date object must be better than 1 millisecond and it just defaults to reporting in ms so we see only 0/1.

Comment 39

[(no longer active)]

(In reply to comment #37)
> 2)  Does webkit (esp. the chromium version) do a sync version of
>     UpdateValueDisplay on value set?  Or do they just mark it as needing to  
>     happen?  Should we just mark it as needing to happen?

I know nearly zero about the webkit code, so I'm not sure how I can determine this, but it's a very good idea.  I think we should do an async UpdateValueDisplay call, I don't see how that might break something, and it should be a bit more efficient.  Do you agree?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Do we want fully async UpdateValueDisplay, or should layout flushes still flush out the value change?

I checked webkit, and they seem to modify the anonymous DOM in their text render object synchronously on value set.  Sounds like we should just make this function faster... ;)  Where was time being spent in frame removal here?

Comment 41

[(no longer active)]

(In reply to comment #40)
> Do we want fully async UpdateValueDisplay, or should layout flushes still flush
> out the value change?

Hmm, I don't know the asnwer to that question.

> I checked webkit, and they seem to modify the anonymous DOM in their text
> render object synchronously on value set.  Sounds like we should just make this
> function faster... ;)

My patch in bug 559710 does that to a great extent.  Still blocking on some accessibility stuff there, but it doesn't look like a big deal.

> Where was time being spent in frame removal here?

What do you mean?  I don't think we spend any time in frame removal, do we?

Comment 42

[Boris Zbarsky [:bzbarsky]]

> What do you mean?  I don't think we spend any time in frame removal, do we?

Comment 11 says:

  38.2% in nsGenericElement::RemoveChildAt, which is almost entirely spent in
        nsFrameManager::RemoveFrame.

Comment 43

[(no longer active)]

(In reply to comment #42)
> > What do you mean?  I don't think we spend any time in frame removal, do we?
> 
> Comment 11 says:
> 
>   38.2% in nsGenericElement::RemoveChildAt, which is almost entirely spent in
>         nsFrameManager::RemoveFrame.

Oh, the RemoveChildAt call itself will be removed with my patch to bug 559710, so this will soon be a moot issue.

Comment 44

[Timothy Nikkel (:tnikkel)]

Using the second testcase without the patch in bug 559970 I get ~117 ms for creating the inputs. With the patch in bug 559970 I get in the range 22-36 ms.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Nice.

Comment 46

[Mounir Lamouri (:mounir)]

All blocking bugs are now fixed. Are we still slow? or can we mark this bug fixed?

Comment 47

[Boris Zbarsky [:bzbarsky]]

Depends on how you measure.  We're not 20x slower anymore.  On my machine, for the first testcase in this bug, I see:

 Minefield with all the patches: 13ms
 Opera: 5ms
 Chrome/Safari: 3ms

For the second testcase, I see (for create/set):

 Minefield: 9/18
 Opera: 16/6
 Chrome: 5/2
 Safari: 4/6

Comment 48

[(no longer active)]

We should probably revisit this bug post 2.0 and do more profiling and see how we can close the gap (or excel other browsers) here.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Why wait post-2.0 to profile?  ;)

That said, Timothy's making ContentStatesChanged not be called as much dropped that 13ms from the first testcase to 10ms today.  That 10ms breaks down as:

  30% under the getElementById quickstub (almost all of this is JS-wrapping the
      node).
  52% under nsTextEditorState::SetValue
      * some copying utf16 to utf8 here
      * some malloc/free traffic
      * 2% IsTooLong
      * 2% HasPatternMismatch
      * 12% under CharacterDataChanged (nsTextFrame::CharacterDataChanged self
            time, FrameNeedsReflow calls,
            nsCSSFrameConstructorCharacterDataChanged self time).
      * 5% BeginUpdate when doing the SetTextInternal from UpdateValueDisplay;
           GetRootPresContext shows up noticeably here.  Perhaps the CSS frame
           constructor should cache that?  Not sure how well that would work
           with tab d&d.
      * 4% allocating the new text fragment storage (?)
      * 2% CharacterDataWillChange
      * 2% EndUpdate
      * 2% QIs in UpdateValueDisplay
      * 2% in UpdateValueDisplay itself

So the only really major hotspot is the JS-wrapping... plus the general "setting text on textnode should be faster" thing (right now 28% of the time is under that, all told).

Comment 50

[Boris Zbarsky [:bzbarsky]]

For the second testcase, first number:

  28% createElement (half under js-wrapping)
  26% appendChild (half BindToTree, the rest beginupdate and contentappended)
  18% setting type (a lot of this is mapped attributes gunk).  Why is the type
      attr mapped???  Shouldn't we just change the return value of
      GetAttributeMappingFunction when the type changes, instead of doing this
      weird check for type in the one mapping function?
   6% is JS Date() calls; might be more on Windows

and some long-tail stuff.  Second number, hard to tell what it times out of the stuff I profiled, but looks like it's almost all under SetValue; the profile for that looks similar to the one in comment 48.

Comment 51

[Boris Zbarsky [:bzbarsky]]

For what it's worth, on current trunk my numbers on the second testcase are something like: 9/6 9/9 10/7 12/8.

With the patch in bug 606528, they're more like:  7/10 8/6 8/7 9/9.  So a bit faster...

Comment 52

[RNicoletto]

With my notebook (Intel Core Duo T7500 - 2 GB RAM - Windows XP SP3) I got this:

Firefox 12
Testcase 1: 11 ms
Testcase 2: 
Stats for Creating input element:
	Average:	0.02 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	15 milliseconds (78.9% of time elapsed during usage)

Stats for Setting input value:
	Average:	0.01 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	9 milliseconds (50% of time elapsed during usage)
	
Internet Explorer 8
Testcase 1: 140 ms
Testcase 2:
Stats for Creating input element: 
  Average: 0.26 milliseconds 
  Max: 16 milliseconds 
  Min: 0 milliseconds 
  Calls: 900 
  Total time used: 235 milliseconds (100% of time elapsed during usage) 
  
Stats for Setting input value: 
  Average: 0.09 milliseconds 
  Max: 16 milliseconds 
  Min: 0 milliseconds 
  Calls: 900 
  Total time used: 77 milliseconds (70.6% of time elapsed during usage) 

Google Chrome 20 (Canary)
Testcasi 1: 0 ms
Testcase 2:
Stats for Creating input element:
	Average:	0.04 milliseconds
	Max:	1 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	32 milliseconds (94.1% of time elapsed during usage)

Stats for Setting input value:
	Average:	0 milliseconds
	Max:	0 milliseconds
	Min:	0 milliseconds
	Calls:	900
	Total time used:	0 milliseconds (NaN% of time elapsed during usage)

I would not say that "Firefox is VERY SLOW" with those testcases.
IMHO this bug has been FIXED.

Comment 53

[David Bolter [:davidb] (NeedInfo me for attention)]

Ehsan, close as WFM?

Comment 54

[(no longer active)]

No, I think even though most of the work here is done, there's still a bunch of stuff we can do to make things even faster, with more profiling and analysis.  It's just that the future improvements would not be as noticeable as, let's say, bug 221820.

Comment 55

[Andrei Purice]

Marking this as Resolved > Worksforme since the issue no longer is reproducible on the latest versions of Firefox Nightly 95.0a1 (2021-10-31), beta 94.0 or release 93.0 on Windows 10. Creation and updating the fields take 2 miliseconds.
If anyone can still reproduce the issue either re-open it or file a new one.