518122 - improve textarea.value+= performance by using "plaintextEditor->InsertText" on only the new part of the string

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Philippe Vouters]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1.3) Gecko/20090909 Fedora/3.5.3-1.fc11 Firefox/3.5.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1.3) Gecko/20090909 Fedora/3.5.3-1.fc11 Firefox/3.5.3

Way to reproduce using provided URL:

Type in the Database field: "provms". Select in the drop box "MySQL".
Type in Username: "firefox", Password: "firefox"
Type in the SQL query "select * from txp_lang". Click on the "Submit" button.

Browsers tested:
Konqueror => OK
Opera (Windows) => OK
Safari (Windows) => OK
Microsoft Internet Explorer 6 SP3 => OK
Seamonkey (orally reported as not OK)

Reproducible: Always

Steps to Reproduce:
1.Type in the Database field: "provms".
2.Select in the drop box "MySQL"
3.Type in Username: "firefox", Password: "firefox"
4.Type in the SQL query "select * from txp_lang"
5.Click on "Submit"
Actual Results:  
Firefox (Linux and Windows) window no longer responding to mouse events after step 5. Linux top utility reports increasing memory and 100% cpu load on firefox.

Expected Results:  
Do not loop and respond to mouse events

Comment 1

[Ria Klaassen (not reading all bugmail)]

Confirmed on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20090922 Minefield/3.7a1pre

I see the hang also with old versions, but the duration has increased from 15 seconds with Firefox 1 to 1,5 minute with Minefield.

Comment 2

[Philippe Vouters]

I am 53 years old. My so far job has been to support developers for computer manufacturers. My latest employer is HP. I work on any OS including Linux using all sort of languages.

I'd like to work on this problem and report my findings and eventual correction proposal. Would you be so kind as to point me to a stable Firefox development branch I can download and start working on my bug report ?

Comment 3

[Ria Klaassen (not reading all bugmail)]

Does this help:
https://developer.mozilla.org/En/Participating_in_the_Mozilla_project

Comment 4

[Philippe Vouters]

CRTL-C gdb to send SIGINT to firefox 3.6 (-->>> 192src/ <<<---).
(gdb) where
#0  0x01c88c55 in nsCOMPtr_base::assign_with_AddRef (this=0xbfffb2f4, 
    rawPtr=0xaa5286c0) at nsCOMPtr.cpp:88
#1  0x013d7917 in nsCOMPtr<nsIMutationObserver>::operator= (this=0xbfffb2f4, 
    rhs=0xaa5286c0) at ../../../dist/include/nsCOMPtr.h:640
#2  0x013d5982 in nsNodeUtils::ContentAppended (aContainer=0xab8dc060, 
    aNewIndexInContainer=242) at nsNodeUtils.cpp:131
#3  0x013c457d in nsGenericElement::doInsertChildAt (aKid=0xaa9042e0, 
    aIndex=242, aNotify=1, aParent=0xab8dc060, aDocument=0xae6e5000, 
    aChildArray=@0xab8dc078) at nsGenericElement.cpp:3243
#4  0x013c422a in nsGenericElement::InsertChildAt (this=0xab8dc060, 
    aKid=0xaa9042e0, aIndex=242, aNotify=1) at nsGenericElement.cpp:3174
#5  0x013c61f5 in nsGenericElement::doReplaceOrInsertBefore (aReplace=0, 
    aNewChild=0xaa904300, aRefChild=0x0, aParent=0xab8dc060, 
    aDocument=0xae6e5000, aReturn=0xbfffb610) at nsGenericElement.cpp:3958
#6  0x013c4e57 in nsGenericElement::InsertBefore (this=0xab8dc060, 
    aNewChild=0xaa904300, aRefChild=0x0, aReturn=0xbfffb610)
    at nsGenericElement.cpp:3493
#7  0x014672e6 in nsHTMLDivElement::InsertBefore (this=0xab8dc060, 
    newChild=0xaa904300, refChild=0x0, _retval=0xbfffb610)
    at nsHTMLDivElement.cpp:56
#8  0x0166f499 in InsertElementTxn::DoTransaction (this=0xaa903c80)
    at InsertElementTxn.cpp:126
#9  0x019b17d5 in nsTransactionItem::DoTransaction (this=0xaa903d00)
---Type <return> to continue, or q <return> to quit---
    at nsTransactionItem.cpp:225
#10 0x019b53b4 in nsTransactionManager::BeginTransaction (this=0xab8e3280, 
    aTransaction=0xaa903c80) at nsTransactionManager.cpp:954
#11 0x019b383e in nsTransactionManager::DoTransaction (this=0xab8e3280, 
    aTransaction=0xaa903c80) at nsTransactionManager.cpp:119
#12 0x0165172a in nsEditor::DoTransaction (this=0xacf3cc50, aTxn=0xaa903c80)
    at nsEditor.cpp:738
#13 0x016531f9 in nsEditor::InsertNode (this=0xacf3cc50, aNode=0xaa904300, 
    aParent=0xab8dc07c, aPosition=242) at nsEditor.cpp:1449
#14 0x016562a5 in nsEditor::InsertTextImpl (this=0xacf3cc50, 
    aStringToInsert=@0xbfffb904, aInOutNode=0xbfffb930, 
    aInOutOffset=0xbfffb92c, aDoc=0xae6e509c) at nsEditor.cpp:2593
#15 0x01646d9e in nsTextEditRules::WillInsertText (this=0xab879a60, 
    aAction=2000, aSelection=0xab8d5980, aCancel=0xbfffbaec, 
    aHandled=0xbfffbae8, inString=0xbfffbb4c, outString=0xbfffba20, 
    aMaxLength=-1) at nsTextEditRules.cpp:771
#16 0x01645a37 in nsTextEditRules::WillDoAction (this=0xab879a60, 
    aSelection=0xab8d5980, aInfo=0xbfffbab4, aCancel=0xbfffbaec, 
    aHandled=0xbfffbae8) at nsTextEditRules.cpp:329
#17 0x01640f28 in nsPlaintextEditor::InsertText (this=0xacf3cc50, 
    aStringToInsert=@0xbfffbb4c) at nsPlaintextEditor.cpp:797
#18 0x0125b5c2 in nsTextControlFrame::SetValue (this=0xab8da69c, 
    aValue=@0xbfffbd08) at nsTextControlFrame.cpp:2701
---Type <return> to continue, or q <return> to quit---
#19 0x012591b7 in nsTextControlFrame::SetFormProperty (this=0xab8da69c, aName=
    0xb7ceb460, aValue=@0xbfffbd08) at nsTextControlFrame.cpp:1909
#20 0x014b261c in nsHTMLTextAreaElement::SetValueInternal (this=0xab879790, 
    aValue=@0xbfffbd08, aFrame=0x0, aUserInput=0)
    at nsHTMLTextAreaElement.cpp:458
#21 0x014b26be in nsHTMLTextAreaElement::SetValue (this=0xab879790, 
    aValue=@0xbfffbd08) at nsHTMLTextAreaElement.cpp:476
#22 0x00f8374a in nsIDOMHTMLTextAreaElement_SetValue(JSContext*, JSObject*, int, int*) () from ./libxul.so
#23 0x004ebdd4 in js_SetSprop (cx=0xb1f68c00, sprop=0xae7531b0, 
    obj=0xab885e60, vp=0xbfffbfa0) at jsscope.h:596
#24 0x004f5e6f in js_NativeSet (cx=0xb1f68c00, obj=0xab885e60, 
    sprop=0xae7531b0, vp=0xbfffbfa0) at jsobj.cpp:4197
#25 0x004d2a5a in js_Interpret (cx=0xb1f68c00) at jsops.cpp:1681
#26 0x004e0ae3 in js_Invoke (cx=0xb1f68c00, argc=1, vp=0xae6d5024, flags=0)
    at jsinterp.cpp:1381
#27 0x004e0ce0 in js_InternalInvoke (cx=0xb1f68c00, obj=0xacf7eb20, 
    fval=-1393038528, flags=0, argc=1, argv=0xae6d5020, rval=0xbfffd8a4)
    at jsinterp.cpp:1436
#28 0x00485357 in JS_CallFunctionValue (cx=0xb1f68c00, obj=0xacf7eb20, 
    fval=-1393038528, argc=1, argv=0xae6d5020, rval=0xbfffd8a4)
    at jsapi.cpp:5134
#29 0x0159d892 in nsJSContext::CallEventHandler (this=0xb1e6ec00, 
---Type <return> to continue, or q <return> to quit---
    aTarget=0xab8b62e0, aScope=0xae6ce020, aHandler=0xacf7eb40, 
    aargv=0xaa9ffe20, arv=0xbfffda90) at nsJSEnvironment.cpp:2097
#30 0x015fd7a0 in nsJSEventListener::HandleEvent (this=0xab8864c0, aEvent=
    0xab8154cc) at nsJSEventListener.cpp:247
#31 0x0141e331 in nsEventListenerManager::HandleEventSubType (this=0xab8a7ee0, 
    aListenerStruct=0xab8a7f04, aListener=0xab8864c0, aDOMEvent=0xab8154cc, 
    aCurrentTarget=0xab8b62e0, aPhaseFlags=6)
    at nsEventListenerManager.cpp:1034
#32 0x0141e7ad in nsEventListenerManager::HandleEvent (this=0xab8a7ee0, 
    aPresContext=0xae6e5c00, aEvent=0xbfffdfe0, aDOMEvent=0xbfffde14, 
    aCurrentTarget=0xab8b62e0, aFlags=6, aEventStatus=0xbfffde18)
    at nsEventListenerManager.cpp:1140
#33 0x0143e8ec in nsEventTargetChainItem::HandleEvent (this=0xb48d4400, 
    aVisitor=@0xbfffde0c, aFlags=6, aMayHaveNewListenerManagers=1)
    at nsEventDispatcher.cpp:244
#34 0x0143eb47 in nsEventTargetChainItem::HandleEventTargetChain (
    this=0xb48d4020, aVisitor=@0xbfffde0c, aFlags=6, aCallback=0xbfffdeb8, 
    aMayHaveNewListenerManagers=1) at nsEventDispatcher.cpp:308
#35 0x0143f203 in nsEventDispatcher::Dispatch (aTarget=0xab8b62e0, 
    aPresContext=0xae6e5c00, aEvent=0xbfffdfe0, aDOMEvent=0x0, 
    aEventStatus=0xbfffe4b8, aCallback=0xbfffdeb8) at nsEventDispatcher.cpp:539
#36 0x011a77c7 in PresShell::HandleEventInternal (this=0xae6e6400, 
    aEvent=0xbfffdfe0, aView=0x0, aStatus=0xbfffe4b8) at nsPresShell.cpp:6459
---Type <return> to continue, or q <return> to quit---
#37 0x011a73c6 in PresShell::HandleEventWithTarget (this=0xae6e6400, 
    aEvent=0xbfffdfe0, aFrame=0xab8da140, aContent=0xab8b62e0, 
    aStatus=0xbfffe4b8) at nsPresShell.cpp:6334
#38 0x014297f6 in nsEventStateManager::CheckForAndDispatchClick (
    this=0xae638fb0, aPresContext=0xae6e5c00, aEvent=0xbfffe694, 
    aStatus=0xbfffe4b8) at nsEventStateManager.cpp:3826
#39 0x014272cf in nsEventStateManager::PostHandleEvent (this=0xae638fb0, 
    aPresContext=0xae6e5c00, aEvent=0xbfffe694, aTargetFrame=0xab8da140, 
    aStatus=0xbfffe4b8, aView=0xae798400) at nsEventStateManager.cpp:2804
#40 0x011a7956 in PresShell::HandleEventInternal (this=0xae6e6400, 
    aEvent=0xbfffe694, aView=0xae798400, aStatus=0xbfffe4b8)
    at nsPresShell.cpp:6480
#41 0x011a736b in PresShell::HandlePositionedEvent (this=0xae6e6400, 
    aView=0xae798400, aTargetFrame=0xab8da140, aEvent=0xbfffe694, 
    aEventStatus=0xbfffe4b8) at nsPresShell.cpp:6317
#42 0x011a6d7f in PresShell::HandleEvent (this=0xae6e6400, aView=0xae798400, 
    aEvent=0xbfffe694, aEventStatus=0xbfffe4b8) at nsPresShell.cpp:6181
#43 0x015909dc in nsViewManager::HandleEvent (this=0xae6a82e0, 
    aView=0xae798400, aPoint={x = 30390, y = 20850}, aEvent=0xbfffe694, 
    aCaptured=0) at nsViewManager.cpp:1211
#44 0x0159094a in nsViewManager::DispatchEvent (this=0xae6a82e0, 
    aEvent=0xbfffe694, aView=0xae798400, aStatus=0xbfffe628)
    at nsViewManager.cpp:1190
---Type <return> to continue, or q <return> to quit---
#45 0x01588e5f in HandleEvent (aEvent=0xbfffe694) at nsView.cpp:167
#46 0x01b738f7 in nsWindow::DispatchEvent (this=0xb4afe9a0, aEvent=0xbfffe694, 
    aStatus=@0xbfffe6ec) at nsWindow.cpp:570
#47 0x01b78ce0 in nsWindow::OnButtonReleaseEvent (this=0xb4afe9a0, 
    aWidget=0xb4d6c920, aEvent=0xaaf3ea60) at nsWindow.cpp:2891
#48 0x01b7f21c in button_release_event_cb (widget=0xb4d6c920, event=0xaaf3ea60)
    at nsWindow.cpp:5554
#49 0x0571ccc8 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#50 0x0019e352 in g_closure_invoke () from /lib/libgobject-2.0.so.0
#51 0x001b3cf0 in ?? () from /lib/libgobject-2.0.so.0
#52 0x001b4ee8 in g_signal_emit_valist () from /lib/libgobject-2.0.so.0
#53 0x001b54e6 in g_signal_emit () from /lib/libgobject-2.0.so.0
#54 0x05850eee in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#55 0x05714128 in gtk_propagate_event () from /usr/lib/libgtk-x11-2.0.so.0
#56 0x057155ea in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
#57 0x0557e92a in ?? () from /usr/lib/libgdk-x11-2.0.so.0
#58 0x05486308 in g_main_context_dispatch () from /lib/libglib-2.0.so.0
#59 0x054899e0 in ?? () from /lib/libglib-2.0.so.0
#60 0x05489b13 in g_main_context_iteration () from /lib/libglib-2.0.so.0
#61 0x01b83f6f in nsAppShell::ProcessNextNativeEvent (this=0xb4b31790, 
    mayWait=1) at nsAppShell.cpp:147
#62 0x01ba169b in nsBaseAppShell::DoProcessNextNativeEvent (this=0xb4b31790, 
    mayWait=1) at nsBaseAppShell.cpp:151
---Type <return> to continue, or q <return> to quit---
#63 0x01ba19ee in nsBaseAppShell::OnProcessNextEvent (this=0xb4b31790, 
    thr=0xb7cd50b0, mayWait=0, recursionDepth=0) at nsBaseAppShell.cpp:296
#64 0x01cddd45 in nsThread::ProcessNextEvent (this=0xb7cd50b0, mayWait=1, 
    result=0xbfffedac) at nsThread.cpp:508
#65 0x01c90578 in NS_ProcessNextEvent_P (thread=0xb7cd50b0, mayWait=1)
    at nsThreadUtils.cpp:230
#66 0x01ba1700 in nsBaseAppShell::Run (this=0xb4b31790)
    at nsBaseAppShell.cpp:170
#67 0x019c2b56 in nsAppStartup::Run (this=0xb4a5f580) at nsAppStartup.cpp:182
#68 0x00ed969e in XRE_main () from ./libxul.so
#69 0x08049706 in main (argc=2, argv=0xbffff484) at nsBrowserApp.cpp:156
(gdb)

Comment 5

[Philippe Vouters]

-->>> cd 191src <<<---
As firefox finishes the job but takes a huge amount of time, I first recompiled all the code using on my Linux system :
$ export CPPFLAGS="-O3 -finline"
This improved somehow the response time. Then I looked at Opera for Linux and my guess is that they use more the pthread technology than firefox. What I would think would be advisable would to use pthread_create to start the javascript interpreter at this stack level:
#28 0x00485357 in JS_CallFunctionValue (cx=0xb1f68c00, obj=0xacf7eb20, 
    fval=-1393038528, argc=1, argv=0xae6d5020, rval=0xbfffd8a4)
    at jsapi.cpp:5134

What is your opinion on this ?

Comment 6

[Ria Klaassen (not reading all bugmail)]

I'm not a coder. Putting some people on the list who can hopefully advise you.

Comment 7

[Philippe Vouters]

minidump_generator.cc, exception_handler.cc, dump_symbols.cc lack the line:
#include <stdio.h>
causing make to fail for both http://hg.mozilla.org/releases/mozilla-1.9.2/ and
 http://hg.mozilla.org/releases/mozilla-1.9.1/

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Profiling this a bit shows that we spend lots of time under nsHTMLTextAreaElement::SetValue.

So I believe the problematic method is 
   function set_textbox(line){
       document.getElementById("textarea").value+=line+"\n";
   } 

Might be better to change
          for (var n=1; n<answer.length;n++){ 
               set_textbox(answer[n]);
          }

to
          var s = "";
          for (var n=1; n<answer.length;n++){ 
               s += answer[n] + "\n";
          }
          document.getElementById("textarea").value = s;


Anyway, I think this is a dup of some other bug.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Jonas, bz, we spend lots of time (43.3%) under nsNodeUtils::ContentRemoved.
Lots of the time goes to Addrefing/releasing nsRange.
nsNodeUtils::ContentAppended takes 18.8%, and nsRange Addref/release is taking
quite a bit there too

Could we add some bit to mutationobserver to indicate whether
the observer wants IMPL_MUTATION_NOTIFICATION to keep strong ref during
notification?

Comment 10

[Philippe Vouters]

Created http://www.x9000.fr/database_query_save.html which contains the original version AND
http://www.x9000.fr/javascript/database_query.html with your proposed javascript change.

Result:

The change DOES improve noticeably the Opera Web browser performances, whereas it is worse with the Firefox Web browser. Indeed, in this bugzilla report we are tackling a problem of Web browsers performances, not functionality.

While running the Linux graphical System Monitoring, it is visible that Opera makes efficient use of my dual core processor whereas Firefox only uses one CPU.

Warmest regards to the Mozila team.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

The actual problem here is ofc the remove/insert. We should just append
text to some text node.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> The actual problem here is ofc the remove/insert. We should just append
> text to some text node.
...In the native anonymous content of textarea.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> We should just append text to some text node.

JS += is always executed as three operations: get, operator+, set.  So the C++ code has no idea that we're appending.

Olli, please file a bug on comment 9 paragraph 2?  If we can do that, it would be really nice.

That all said, on the testcase in the url field, using the steps from comment 0, I see no more than a second of lag after clicking "Submit".  This is using Firefox 3.5.3 release build on Mac.

Ria, Philippe, any ideas on what I need to do differently to reproduce the bug?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Just tried this on Firefox 3.5.3 on Linux as well (in a VM).  No noticeable non-network lag after clicking Submit; no UI freeze.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #13)
> JS += is always executed as three operations: get, operator+, set.  So the C++
> code has no idea that we're appending.
Oops, right.
Though we could perhaps compare the current text and the new text and
if new text starts with the current, do just some append operation.

Comment 16

[Boris Zbarsky [:bzbarsky]]

We could, yeah.  Need to get rid of the <br> mess first.

I'd still like to know how I can reproduce anything resembling a minute-long (or heck, 15-second-long) hang...

Comment 17

[Philippe Vouters]

Dear Firefox maintainers,

Here are my test conditions:

Running Fedora 11 up to date. Built http://hg.mozilla.org/releases/mozilla-1.9.1/ with the following conditions:
$ export CPPFLAGS="-O3 -finline"
$ export JAVA_HOME=/usr/local/jdk1.6.0_04/
$./configure --enable-application=browser --disable-libnotify
$ make

Running: 
$ /home/philippe/191src/dist/bin/firefox http://www.x9000.fr/javascript/database_query.html

Testing in parallel Opera 10.0 for Linux.

With both browsers and BEFORE clicking on the "Submit" button, I activate on my Gnome Desktop Applications->System Tools->System Monitor to graphically view how my dual-core CPU's are used.

What is visible is that Firefox long time uses (100% load) only one CPU. Another visible effect is that the Firefox window no longer responds to mouse scrolling wheel button until it has finished its job. This is true using the http://www.x9000.fr/javascript/database_query.html (your proposed javascript change) or http://www.x9000.fr/javascript/database_query_save.html (original code as submitted).

What is visible is that Opera uses the two processors during a short time. While Opera is busy feeding the text in the "textarea", Opera window still anwers to mouse wheel movements. When using the submitted javascript code http://www.x9000.fr/javascript/database_query_save.html, Opera does respond immediately to the textarea vertical scrollbar sollicitation. Using your proposed javascript change, Opera takes almost no time to answer to the textarea vertical scrollbar sollicitation.

Hoping this will help,
Warmest regards to the team.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Philippe, I'm testing on FC9, using the build from here: <ftp://ftp.mozilla.org/pub/firefox/releases/3.5.3/linux-i686/en-US/firefox-3.5.3.tar.bz2>.  Using this build, there is no more than 1-2s of unresponsive time, if that (hard to tell, since vmware itself doesn't respond particularly snappily).  I assume you're seeing a much longer time than that?  I'm testing on <http://www.x9000.fr/javascript/database_query.html>, since the other URI you cite gives me a 403 error.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

http://www.x9000.fr/javascript/database_query.html is fast here and the
old test doesn't seem to work anymore (as bz noted).

Comment 20

[Philippe Vouters]

http://www.x9000.fr/javascript/database_query_save.html now responds correctly. There was a problem of file protection preventing Apache to access the file.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Now I do see the problem, with the original testcase.  I don't see the regression Ria reports, though.  This certainly needs bug 240933 fixed; once that's done we should retest.

Comment 22

[Philippe Vouters]

I loose my comments with collisions.

Great news ! I must dig into my Linux to understand what happened.

/home/philippe/191src/dist/bin/firefox http://www.x9000.fr/javascript/database_query.html responds almost immediately with very low CPU overhead.

/home/philippe/191src/dist/bin/firefox http://www.x9000.fr/javascript/database_query_save.html does shows up the problem.

Great job you did guys ! You did help me tune Internet browsers response time. This is one service that may sell Mozilla.org along with proposing freewares : how to best tune Web pages to get the minimum network and browser CPU load.

Comment 23

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #21)
> I don't see the regression Ria reports, though.

With the URL http://www.x9000.fr/javascript/database_query_save.html there was a long hang that started a few seconds after the query result was received. With old builds the hang was much shorter.

http://www.x9000.fr/javascript/database_query.html shows indeed no hang.

This bug can't stay in the FIXED state, WORKSFORME is also not correct, because the problem is still there with the original URL which was the reported bug.

Philippe, does the original URL stay on the server so that it can be tested once bug 240933 is fixed?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ria, does the hang go back to the smaller value for you if you disable spellchecking?  I see the same thing you do (hang after the value is filled in), which is rather confusing to me...

Comment 25

[Boris Zbarsky [:bzbarsky]]

Another interesting question here is why the spellchecking is happening sync (as it appears to be).  Anyone know why?

Comment 26

[Philippe Vouters]

You may use both links with the provided Username/password information. One silly thing : I must learn how to restrict access rights from MySQL to enable readings but not modifications rights to this database.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

A test patch to append only new data (and not re-set everything) decrease used time from 68s to 3s

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

After that kind of patch the main bottleneck is GetValue (~60%). I think we could
cache the return value in common cases.

Comment 29

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #24)
> Ria, does the hang go back to the smaller value for you if you disable
> spellchecking?  I see the same thing you do (hang after the value is filled
> in), which is rather confusing to me...

This makes indeed difference; with spell checking enabled it takes 70 seconds and without spell checking 35 seconds.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP, not properly tested

This and the patch for bug 518836 drops the time from 65s to 0.5s.
But this isn't well tested yet.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better

Comment 32

[Philippe Vouters]

Olli, just a comment from a 53 years old developer support person. You are certainly much more skilled than currently I am with the Firefox code. However, the method I use to improve the code is to run a profiler and consider where I do loose most of the time. This is where I focus my efforts. Simply observing the code, extracting as relevant and as tiny part of the code as possible in a smaller test case let me better understand where the actual problem is and how I must rewrite this code part to get better performances.

The javascript simple rewrite you proposed me was indeed excellent and is one approach to the problem from the user end. As far it seems from your attachment in Comment #31, you seem to have added extra-complexity to the existing code. I suggest you extract from Firefox code the minimum of involved routines as you can after having well observed using a profiler. The smallest your extract from the Firefox code will be, the most efficient way you'll work. And you'll imagine with only very small pieces of code how you can keep the entire functionality just rewriting some statements or the whole part differently.

This said, someone pointed out a relevant word : he mentioned Firefox work is sync where he was thinking it was async. If you observe Opera/Linux in detail using the problematic URL, you'll notice like me that when using the scroll wheel of the mouse to scroll the entire Opera window, Opera follows the movement (this is async). If you attempt to scroll the textarea still using the problematic URL and before Opera finishes to feed it, you'll notice you can't immediately (this specific work by Opera is completely sync). In a future Firefox, you must imagine how you can do essentially async work on every item. With the advent of multi-cores CPU's, an advise I can do is to use as most as concurrent programming as you can. These task will run on virtual processors which may at one time correspond to physical processors. Physical processors is not your concern. You must concentrate on virtual processors. The more physical CPU's you have the fater your code will be. As I so far worked during my career, my problem is no longer to split a problem into sequential procedures, but into concurrent tasks. I do not know if this is actually feasible for a GUI tool such as Firefox with the constraint of graphics (X lib especially). If you succeed, you'll get a terribly powerful Firefox.

Olli, I'd like to open you my private knowledge database. It is voluntarily non public as there is at least 5000 hours of personal overtime work in it. There are a couple of articles of mine which treat about software and performances. One is especially hardware processors architecture oriented using two processors that HP sell (Alpha and Itanium). However the conclusions drawn are as well true on any modern processor including my personnal low cost Celeron Dual Core. I'll send you mail with your username and password to access my articles. Do take time to have a look at it. For you to not loose too much time on articles of no current interest for you, I suggest you use the local search engine at http://vouters.dyndns.org:8080/ entering words of immediate interest for you such as "profiler or performance" with a Boolean query. Then from the text ala Google the search engine returns, you may find extremely relevant technical articles of mine that you may eventually apply to this Firefox problem. You'll find a Cobol performance killer code using both an Opensource Cobol and an HP Cobol product and how I analysed the problem to detect and prove where the performance bottleneck resides.

If you wish to know more about concurrent programming, enter the query "pthread or concurrent". The OpenSSL pthreaded server you'll find there is especially commented to fully explain this technology with its programming limits. For your curiosity, if I remember well, you ought to be able to use Firefox and get the answer from this OpenSSL pthreaded server. All this provided code has its own Microsoft Windows translation included in the article.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #32)
> However,
> the method I use to improve the code is to run a profiler and consider where I
> do loose most of the time.
This is what I'm doing here; profiling the testcase using Shark and optimizing
problematic parts in Gecko.

> As far it seems from your attachment
> in Comment #31, you seem to have added extra-complexity to the existing code.
Well, actually in this case the complexity isn't too bad.
Just simple value cache and a mechanism to append text when += is used.
Once bug 240933 is fixed, we could hopefully remove some of these optimizations.
But at least the "append text" part should stay even after bug 240933. 
And that is the main bottleneck here.

> In a future
> Firefox, you must imagine how you can do essentially async work on every item.
> With the advent of multi-cores CPU's, an advise I can do is to use as most as
> concurrent programming as you can. These task will run on virtual processors
> which may at one time correspond to physical processors.
Async doesn't mean that the processing can happen in multiple threads (which
then multiple CPU cores can execute). But sure, making things async when possible helps often. See for example bug 518836, which this bug depends on.

> If you wish to know more about concurrent programming, enter the query "pthread
> or concurrent".
If you're interested in Firefox's plans for "parallel browsing",
I suggest you to read about Electrolysis project.
Another interesting link is http://parallelbrowser.blogspot.com/2007/09/hello-world.html

Anyway, this kind of discussion doesn't belong to a bug, but to emails or
newsgroups (http://groups.google.com/group/mozilla.dev.performance).

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Attachment: async reflow

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Attachment: async reflow

Comment 36

[Philippe Vouters]

I completely agree with Comment #33. First there is no bug, there is just a complete design issue with today's browsers. This "bug" report is just a problem of performance. The richer a today's browser is (such as Firefox), the more it is subject to such phenomenon. The URL link you mentioned (http://parallelbrowser.blogspot.com/2007/09/hello-world.html) describes accurately what is visible here. If you refer to the very long gdb "where" I posted above there is too much sequential code running, hence the performance penalty, especially noticeable with Firefox. Opera being more parallel less suffer from the problem. However, even with Opera and from a programmer minded end-user, you can notice they did not succeed to paralellise all items.

So I strongly agree upon closing this "bug" report and I suggest you move or duplicate onto other appropriate places all this content as a reference for upcoming Firefox directions.

Why my warmest regards to everyone.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Olli, I'd really prefer we didn't lump in the async reflow change (which is very regression-prone, imo) with other changes.  Let's do that one in bug 259636.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Sure.
I think I'll split the patch to valuecache part and
append part, and can leave reflow part to bug 259636.
But there are still few issues to solve first (like could we
fix bug 240933 soonish).

Comment 39

[Boris Zbarsky [:bzbarsky]]

> like could we fix bug 240933 soonish

Once we fix the bugs blocking it...

Comment 40

[Biju]

I am eager to see this landed in trunk, 
when we expect it?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Just working on it again.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +some tests and limit cache to textareas

I posted this to tryserser.
Drops the testcase from ~70s to ~700ms

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Unfortunately the patch fails in a print preview test.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v8

I wish we could get rid of the non-primary print/printpreview presentation shells.  So, I can clean up this a bit after clone-doc-for-printing.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 411754 [details] [diff] [review]
v8

Boris, what do you think about this.
A bit ugly, but textcontrolframe isn't the cleanest code I've seen :(

I posted this to tryserver, hoping it passes everything this time.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 411754 [details] [diff] [review]
v8

>+++ b/content/html/content/src/nsHTMLTextAreaElement.cpp
> nsHTMLTextAreaElement::Reset()
>+    SetValue(EmptyString()); // Really reset the form control frame.

Why is this needed?  Might be worth a longer comment explaining why just SetValue(resetVal) won't do the right thing...  I'd like to understand why not, for example.

>+++ b/layout/forms/nsTextControlFrame.cpp
>@@ -1549,17 +1552,20 @@ nsTextControlFrame::InitEditor()
>+    // We really need to update the editor, because otherwise print preview
>+    // wouldn't get the right value, since the value is also in
>+    // normal presentation.
>+    SetValue(defaultValue, PR_TRUE);

Is this still relevant?

>+nsTextControlFrame::SetValue(const nsAString& aValue,
>+    // GetText removes newlines from single line control.

Did we actually use to get the same string twice in two different ways?  :(

As long as you're here, GetText is not actually an interface method, right?  Could we just make it return void and take a string ref, not a pointer?  Followup bug, probably.

Also, IsPlainTextControl should go away.  Again, followup bug.

Once we nix <br>s we can simplify all this code a whole bunch....

>@@ -2650,17 +2678,31 @@ nsTextControlFrame::SetValue(const nsASt
>+        if (!currentLength ||
>+            currentLength > newlength ||
>+            !currentValue.Equals(Substring(newValue, 0, currentLength)) ||

Why not just:

   if (!currentLength ||
       !StringBeginsWith(newValue, currentValue) ||

?  Ends up being the same code, but no point rewriting it here.

>+        const nsAString& insertValue =
>+          Substring(newValue, currentLength, newlength - currentLength);

Maybe:

  StringTail(newValue, newlength - currentLength)

?  Either way; the StringTail likely does an extra subtraction, but seems more readable to me...

r=me with the nits above addressed and the aForceEditorValueUpdate thing either excised or explained better.

Is there a reason to not go ahead and also cache the value for single-line text controls, by the way?

Comment 47

[(no longer active)]

I'm planning to fix bug 240933 soon.  If this patch isn't targeted for anything <= 1.9.2.x, then is this worth taking before bug 240933 is fixed?

I'm mostly curious because this conflicts with my work in bug 534785.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

How much does this conflict with bug 240933?

The caching part could be done differently if textarea contained only
<div>#textnode</div>, but is that really conflicting badly with bug 240933?

I need to update the patch anyway to remove the print preview related hack
(shouldn't be needed now that we clone the document for printing).

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #46)
> (From update of attachment 411754 [details] [diff] [review])
> >+++ b/content/html/content/src/nsHTMLTextAreaElement.cpp
> > nsHTMLTextAreaElement::Reset()
> >+    SetValue(EmptyString()); // Really reset the form control frame.
> 
> Why is this needed?  Might be worth a longer comment explaining why just
> SetValue(resetVal) won't do the right thing...  I'd like to understand why not,
> for example.
I had forgotten that. It is there so that spellchecking gets reseted.
Needed to get the tests for bug 300691 pass.
So, that hack is not optimal. I need to figure out some other way to
fix spellchecking. (the bug isn't visually too bad, just a minor uncontinuity
in the misspelling underline.)

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Ok, I think I can fix the spellcheck problem without fixing bug 537230.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v10

This has still the EmptyString(). Seems like it is needed there to get the
initial spellchecking. (or perhaps we could change spellchecking behavior.)

But the discontinuity should be fixed by changing selection handling a bit.

Will ask re-review later.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 419583 [details] [diff] [review]
v10

Added a comment about the EmptyString() usage (hopefully that is enough).
Other option would be to change spellchecker, but this approach seems to work well.


Changed SelectAllOrCollapseToEndOfText to re-get child before
eText check.

Removed the hack for print preview.

Comment 53

[(no longer active)]

(In reply to comment #48)
> How much does this conflict with bug 240933?
> 
> The caching part could be done differently if textarea contained only
> <div>#textnode</div>, but is that really conflicting badly with bug 240933?

Actually, this conflicts with my work in bug 534785, not bug 240933.

But my main question was, given an imaginary world in which bug 240933 were fixed, would we still want to take this patch on top of that?  :-)

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Yes, we want to append when possible, not replace. So even after bug 240933 we should take this patch, though caching the current value might not be needed.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419583 [details] [diff] [review]
v10

r=bzbarsky; sorry for the lag...

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/872dcf34dab3