35296 - SetCaretEnabled() takes too long.

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Joe Francis]

One text pastes, nsIPreShell::SetCaretEnabled() takes a lot of time, and it 
appears to be proportional to the size of the paste.

Comment 1

[rubydoo123]

m19

Comment 2

[rubydoo123]

please do not nominate for nsbeta3 -- this will be addressed after crashes, 
regressions, correctness and polish bugs have been resolved, all Composer perf 
bugs have been set to m19/m20

Comment 3

[Blake Ross]

*SPAM*: Changing the QA contact of all open/resolved Selection bugs from 
[e-mail] to [e-mail].  After the many great years of service 
Eli has given to Mozilla, it's time for him to move on; he has accepted a 
position at Eazel.  We'll be sad to see him go, and I'll do my best to fill his 
spot...

Comment 4

[rubydoo123]

reviewed by Bijal and beppe, setting to nsbeta3+, p2

Comment 5

[selmer (gone)]

If the paste of a normal sized paragraph is reasonble, this is not a P2.  If 
even that case is unreasonable, please renominate with timing data in the bug.

Comment 6

[rubydoo123]

pdt deems this not necessary for beta3, setting it to p3 which places it below
the wire for beta and rtm, marking future and adding helpwanted. If this impacts
your work or impacts another bug, please state your arguement in the bug, delete
entries in the whiteboard for reconsideration.

Comment 7

[Jaime Rodriguez, Jr.]

Removing adequated PDT grafitti.

Comment 8

[Blake Ross]

changing selection qa to tpreston.

Comment 9

[rubydoo123]

removing myself from the cc list

Comment 10

[leon.zhang]

It is an  very serous perf bug in fact.

caret module consumed so much CPU time, It is not allowed in many env, especaily
for all multiuser systems. Any other system like remote X-sessions, SunRays,
Linux terminal servers, and (since this bug is OS independent) also Windows
Based terminals in Terminal server environments like Citrix Metaframe are
affected by this bug. 

At same time, this bug blocked bug 188318

Comment 11

[leon.zhang]

PresShell::SetCaretEnabled ---> nsCaret::SetCaretVisible

Comment 12

[leon.zhang]

According to testing quantify data of bug 188318, time consumed in function:
nsCaret::SetCaretVisible mainly focused in calling function:  
nsCaret::StartBlinking/StopBlinking --> nsCaret::DrawCaret

When calling nsCaret::DrawCaret, time mainly consumed in function:
nsTextFrame::GetPointFromOffset, when user typed text quickly in input field.

so function nsCaret::DrawCaret and nsTextFrame::GetPointFromOffset can be
regarded as an target of improving perf.

Comment 13

[leon.zhang]

Function nsTextFrame::GetPointFromOffset compute the coordinates of the caret.

When user type a char, this function was called many (3 - 6) times, so cache the
result of this function can improve the perf of composer.

I make a test according to thought above:
1) allocate a big array in textframe, and each elment cache the result of caret
of the character typed.
2) If call this function at the first time, save the result of this function;
else use the cached  value.

The result is exciting, the percent value of cpu usage decreased from orignal 
63 ~ 75% to current 50~57%, especally when scroll window resulted from typing
character.

test env: trunk20030112 + win2000

Comment 14

[leon.zhang]

thanks simon for giving 2 sugggestons about this bug:
 1. reduce the call count of SetCaretEnabled
 2. make nsTextFrame::GetPointFromOffset faster

Pervious experiment can verify the second suggestion through caching the
valuable computing result.

Comment 15

[leon.zhang]

Attachment: test result of caching function: nsTextFrame::GetPointFromOffset

you can find from the Image that there is a increase of using cpu in the latter
part of the curve of CPU usage, compared with the first part of the curve.

In the first part of the curve, I set a buff which can save the position of
caret, and composer can get needed result without computing them again.

If the buff is full(the sceond part of curve), then mozilla have to compute
position vaules again and again, so lead to more uasge of CPU. 

btw,  I set a small composer window to force scroll window.

Comment 16

[leon.zhang]

Attachment: codes caching result of function: nsTextFrame::GetPointFromOffset(for discuss)

there are still many things to do. 
It will be helpful if you can give suggestions.
:)

Comment 17

[Simon Fraser [no longer active]]

can you reattach the image in JPEG or PNG format? We can't all display BMPs.

Comment 18

[leon.zhang]

Attachment: test result of caching function: nsTextFrame::GetPointFromOffset

test result of caching function: nsTextFrame::GetPointFromOffset

Comment 19

[Simon Fraser [no longer active]]

Attachment: test patch to cache the frame offset in the caret code

Comment 20

[leon.zhang]

patch above not works for previous test case, because function:
nsCaret::GetCaretCoordinates never been called.

Becuse target should be DrawCaret, so codes below should be moved to function:
nsCaret::DrawCaret() (???????)
+  if (!mCachedOffsetValid) {
+    theFrame->GetPointFromOffset(presContext, rendContext, theFrameOffset,
&mCachedFrameOffset);
+    mCachedOffsetValid = PR_TRUE;
+  }
+  
+  nsPoint framePos = mCachedFrameOffset;

Comment 21

[leon.zhang]

btw, nsCaret::ClearFrameRefs also has not been called.

Comment 22

[Simon Fraser [no longer active]]

Oops, comment 20 is correct. There are 2 callers of GetPointFromOffset in the
caret code, and I changed the wrong one. Does putting that in DrawCaret make a
difference?

Comment 23

[leon.zhang]

Attachment: Deatailed info about time consumed when calling nsCaret::DrawCaret

Comment 24

[leon.zhang]

Attachment: detailed time info about function: nsTexFrame::GetPointFromOffset

Comment 25

[leon.zhang]

functions consumed most of the time when calling them:

nsTexFrame::GetPointFromOffset(9.01% of whole app)
--> nsTexFrame::PrepareUnicodeText(85.10% of caller)
--> nsTextTansFormer::GetNextwWord(76.14% of caller)
--> nsTextTansFormer::ScanNormalAsciiText(99.25% of caller)

Comment 26

[leon.zhang]

further detailed info:

   nsCaret::DrawCaret(8.78% of whole app, 78.90% call below)
1) nsTexFrame::GetPointFromOffset(76.89% from above,23.11% from below, 9.01% of
whole app)  *****
   nsTypedSelection::GetPointFromOffset(2.22% of whole app, 93.78 % call above)


1) nsTexFrame::GetPointFromOffset(9.01% of whole app,8 5.10% call below) --> 
2) nsTexFrame::PrepareUnicodeText( 56.45% from above and 43.51% from below,
13.58% of whole app)  ***** 
<--nsTextFrame::PaintAsciiText(8.86% of whole app, 66.24% call above)


2) nsTexFrame::PrepareUnicodeText(13.58% of whole app, 76.14% call below) --> 
3) nsTextTansFormer::GetNextwWord(63.12% from above, 36.88% from below, 16.37%
of whole app) *****  
<-- nsTextFrame::MeasureText(10.66% of whole app, 56.65% call above, 42.53% call
nsRenderingContextGTK::GetTextDimensions, %100 from below) 
<-- nsTextFrame:reflow(11.29% of whole app, 94.24% call above)


3) nsTextTansFormer::GetNextwWord(16.37% of whole app, 99.25% call below)  --> 
4) nsTextTansFormer::ScanNormalAsciiText(100% from above, 16.25% of whole app) *****

Comment 27

[leon.zhang]

Attachment: test patch(cache info in nsTextFrame, increase additional 12 bytes)

Not set a array for position of caret like attachment 114089 [details] [diff] [review], still can get the
same result as it.

Comment 28

[Simon Fraser [no longer active]]

Comments on nsTextFrame::PrepareUnicodeText() and nsTextTransformer::GetNextWord():

* content gets copied twice in process: nsTextTransformer copies it from content
  into its mTransformBuf, and nsTextFrame::PrepareUnicodeText then copies
  it again into its nsAutoTextBuffer.

* nsTextTransformer has a NS_TEXT_TRANSFORMER_TRANSFORMED_TEXT_IS_ASCII flag,
  but this is only set when PrepareUnicodeText is called from nsTextFrame::Reflow.

Comment 29

[leon.zhang]

http://bugzilla.mozilla.org/show_bug.cgi?id=188318#c40

Not very sure width of composer is related to perf of Caret process.

Comment 30

[leon.zhang]

Attachment: a little modification to attachment 114162 (based on prevous comments)

Although caching info in nsCaret is not better than doing it in nsTextFrame, it
still helpful to imporve perf of caret process.

Comment 31

[leon.zhang]

Attachment: new version(according to suggestions of smfr)

Comment 32

[leon.zhang]

some testing results of the newest patch:

nsPreShell::                  nsCaret::               nsCaret::
SetCaretEnabled               SetCaretVisible         DrawCaret

10.58%                         14.89%                  8.42%       without patch
--------------------------------------------------------------------------------
7.69%                          12.10%                  4.50%       applied patch 
6.54%                          11.26%                  4.67%


typed 1000 chars in a line without wrapping line

soloris 8 + trunk20030212

when composer startup, begin record data using gdb, then
record data untill the typed  chars amount to 1000.

Note: 
    percent value of table above is the percent value of the time consumed by
whole app (percent of root).

Comment 33

[leon.zhang]

btw, testing data in above comments is produced by means of rational quantify.

Comment 34

[leon.zhang]

thx smfr

Comment 35

[leon.zhang]

a new test based on current patch:

  After applied current patch, comment out code :"StCaretHider
aretHider(caret);" in nsSlection.cpp and nsEditor.cpp just like the fisrt part
of  patches for bug 54153.
   This can prevent codes related to selection and editor call caret process.  

test data:

nsPreShell::              nsCaret::               nsCaret::
SetCaretEnabled           SetCaretVisible         DrawCaret

10.58%                    14.89%                  8.42%       without patch
--------------------------------------------------------------------------------
7.69%                     12.10%                  4.50%       applied patch 
6.54%                     11.26%                  4.67%
---------------------------------------------------------------------------
8.68%                     8.68%                   4.70%       comment out 
8.96%                     8.95%                   4.77%     "StCaretHider. .."

Comment 36

[leon.zhang]

file a new bug (http://bugzilla.mozilla.org/show_bug.cgi?id=194343) for
following optimization

Comment 37

[leon.zhang]

checked in by henry.jia.

thx smfr, henry.

further optimization will be done in bug 194343.

Comment 38

[Simon Fraser [no longer active]]

There may be DOM manipulations that can be made in 'browse the caret' mode that
can break this (cause the caret to show in the wrong place). It needs some
testing via JS that messes with text nodes, when browsing with the caret.

Comment 39

[Simon Fraser [no longer active]]

This probably caused bug 194774.

Comment 40

[kinmoz]

I just verified that the patch for this bug did cause bug 194774. I commented
out the line:

  mCachedOffsetValid = PR_TRUE;

in nsCaret.cpp and things started working fine again.

Comment 41

[kinmoz]

Reopening this bug. I checked in the patch for bug 194774 for leon which
disables the caching code landed for this bug.

Comment 42

[leon.zhang]

When user type backspace, nsCaret::DrawCaret will call 
nsTextFrame::GetPointFromOffset (mCachedOffsetValid have been set NS_FALSE) ,
whose return value is not correct and mCachedFrameOffset is always (0,0).
So nsTextFrame::GetPointFromOffset cannot give us a correct result.

---------old comments from bug 197774 's cmments #8

When comment out that codes like comments #40, we can get the correct result.

--------- from comments #40


 nsTextFrame::GetPointFromOffset calls nsTextFrame::PrepareUnicodeText.

I find that If PrepareUnicodeText canot filled the indexbuff with correct value,
GetPointFromOffset will not compute correctly.

At the same time,  nsTextFrame::PrepareUnicodeText can get the correct value if
mContentLength is set the factual length of current content.

In addition, when nsTextFrame::Reflow is called, mContentLength can be set the
correct value(mContentLength = textData.mOffset - startingOffset;).

In a word,  when the content changes, the frame cannot synchronize quickly, and
lead to this regression. we should do this to ensure the correctness of caret
process.

I make a simple test, it can works for BACKSPACE.
I will give this test patch quickly.

Comment 43

[leon.zhang]

Attachment: test patch for regression

Comment 44

[Simon Fraser [no longer active]]

Calling presShell->FlushPendingNotifications() there may cause CPU usage to go
up when typing. You need to be careful about its performance implications.

Comment 45

[kinmoz]

Comment on attachment 115631 [details] [diff] [review]
test patch for regression

I'm going to obsolete this patch because FlushPendingNotifications() should
never be called in this manner from any editor method.

I'm guessing the frame data is out of date because
nsEditor::EndUpdateViewBatch() ends selection batching before it ends paint and
reflow batching. It should end the batches in the reverse order used in
nsEditor::BeginUpdateViewBatch(). (end reflow, end view, end selection)

That all said, I think the caching scheme, as it is implemented right now, may
still have problems since as sfraser pointed out above we may still be able to
see these types of problems in the browser when caret browsing is turned on,
and DOM/JS code is used to manipulate the document. Note that when manipulating
the document in this manner in the browser, *no* selection/paint/reflow
batching is done.

Also I think that the caching doesn't take into consideration that even though
the caret's (content, offset) position hasn't changed that it's geometrical
position can still be different. Have you tested the case where you have
something like this:

    Test M|

where '|' is the caret, and some JS is used to replace the 'M' with an 'i'
which is much narrower? Does the caret draw in the correct spot?

What about the case where the caret is in a text node, and some JS inserts
something (like an image or another text node) before the text node containing
the caret, which changes it's geometrical position?

Comment 46

[leon.zhang]

Main function calling  sequence when user type a BACKSPACE:

nsAutoPlaceHolderBatch()

nsAutoRules() -> nsHTMLEditRules::BeforeEdit  -> ... -> DrawCaret
~nsAutoRules() -> nsHTMLEditRules::AfterEdit  -> ... -> DrawCaret


StCaretHider()  ... -> DrawCaret
EndReflowBatching (NOTE: After this, we can ensure info of content map to frame )
~StCaretHider()  ... -> DrawCaret

~nsAutoPlaceHolderBatch()
-> nsEdiotor:: EndPlaceHolderTransaction ...-> StCaretHider() ... -> DrawCaret
                                               ~StCaretHider() ... -> DrawCaret
-> nsEdiotor:: ScollSelectionIntoView     ...-> StCaretHider() ... -> DrawCaret
                                               ~StCaretHider() ... -> DrawCaret

Comment 47

[leon.zhang]

from comments #46, we will find:

1) we have cacalulated an position of caret once in  nsHTMLEditRules::AfterEdit.
   Because it happend before reflow, we can not ensure its value is correct.

2)At the same time, in the patch lead to regression, we have cached the
questionable value. In th latter function calling DrawCaret, we have used this
cached value, we have no oppertunity to revover from our mistake.  
so .... regression happend!

Comment 48

[kinmoz]

I think we need to remove the caret hiding used in AfterEdit() ...  all our
Editor entry points should probably use nsAutoUpdateViewBatch variables to
ensure proper reflow/paint/selection ordering.

Comment 49

[leon.zhang]

Attachment: new patch

Comment 50

[leon.zhang]

Kin, you are right :)
I agree with you, see my analysis :)

btw, I think also we should not that function in nsAutoRules:
1) it is before reflow, compute an questionable value.
2) calling time-consumimg nsTextFrame::PrepareUnicodeText, lead to perf of
editor.

Comment 51

[Simon Fraser [no longer active]]

I've been thinking that maybe the caret code needs to ask the nsIPresShell if
there are any pending reflows, before caching stuff. Otherwise we might run into
this again.

Comment 52

[Simon Fraser [no longer active]]

Comment on attachment 115952 [details] [diff] [review]
new patch

Joe needs to look at this.

Comment 53

[leon.zhang]

btw, the incorrect caret pos (its value always 0,0) from
nsHTMLEditRules::AfterEdit has leaded to caret's flying around during editing.

Comment 54

[Simon Fraser [no longer active]]

Attachment: Testcase for the patch

Comment 55

[leon.zhang]

Comment on attachment 116605 [details]
Testcase for the patch

current patch dose not work for this testcase :(

Comment 56

[leon.zhang]

Two solutions to this test case:
1) we can judge whether the "accessibility.browsewithcaret" is TRUE, if yes, we
do not use this cached result, else use cache mechanisms. 
I just change only codes in nsCaret.cpp and it works.

2) we can judge whether content of textframe have been changed or notify the
nsCaret when content changed, so that we can compute new pos of caret even
though the content offset and textframe are unchanged.

   DOM/content ---> presshell --> Caret
or 
   textframe  ---> DOM/content


Comparison:
  the first solution is simple, and need change a little codes  of nsCaret.cpp.
the last one solution can resolve completely, but need to changed more codes in
different moodules.

Comment 57

[leon.zhang]

second solution:

Index: base/public/nsICaret.h
===================================================================
RCS file: /cvsroot/mozilla/layout/base/public/nsICaret.h,v
retrieving revision 1.28
diff -u -r1.28 nsICaret.h
--- base/public/nsICaret.h	5 Mar 2003 15:08:23 -0000	1.28
+++ base/public/nsICaret.h	10 Mar 2003 05:56:33 -0000
@@ -125,6 +125,7 @@
    **/
   NS_IMETHOD DrawAtPosition(nsIDOMNode* aNode, PRInt32 aOffset) = 0;
 
+  NS_IMETHOD DisableCache() = 0;
 };
 
 nsresult
Index: base/src/nsCaret.h
===================================================================
RCS file: /cvsroot/mozilla/layout/base/src/nsCaret.h,v
retrieving revision 1.34
diff -u -r1.34 nsCaret.h
--- base/src/nsCaret.h	27 Feb 2003 23:08:23 -0000	1.34
+++ base/src/nsCaret.h	10 Mar 2003 05:56:33 -0000
@@ -64,6 +64,7 @@
     NS_IMETHOD    SetCaretWidth(nscoord aPixels);
     NS_IMETHOD    SetVisibilityDuringSelection(PRBool aVisibility);
     NS_IMETHOD    DrawAtPosition(nsIDOMNode* aNode, PRInt32 aOffset);
+    NS_IMETHOD    DisableCache();
 
     //nsISelectionListener interface
     NS_IMETHOD    NotifySelectionChanged(nsIDOMDocument *aDoc, nsISelection
*aSel, short aReason);
Index: base/src/nsCaret.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/base/src/nsCaret.cpp,v
retrieving revision 1.105
diff -u -r1.105 nsCaret.cpp
--- base/src/nsCaret.cpp	27 Feb 2003 23:08:18 -0000	1.105
+++ base/src/nsCaret.cpp	10 Mar 2003 05:56:37 -0000
@@ -445,6 +445,12 @@
   return NS_OK;
 }
 
+NS_IMETHODIMP nsCaret::DisableCache()
+{
+  mCachedOffsetValid = PR_FALSE;
+  return NS_OK;
+}
+
 
 #ifdef XP_MAC
 #pragma mark -
Index: html/base/src/nsPresShell.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v
retrieving revision 3.611
diff -u -r3.611 nsPresShell.cpp
--- html/base/src/nsPresShell.cpp	6 Mar 2003 23:06:48 -0000	3.611
+++ html/base/src/nsPresShell.cpp	10 Mar 2003 05:56:54 -0000
@@ -5168,6 +5168,9 @@
   VERIFY_STYLE_TREE;
   DidCauseReflow();
 
+  if (mCaret)
+    mCaret->DisableCache();
+
   return rv;
 }

Comment 58

[leon.zhang]

About second solution:
1) When the textnode changes, function: nsGenericDOMDataNode::ReplaceData() will
be called.  That function will activiate reflow process through
PresShell::ContentChanged() .

nsGenericDOMDataNode::ReplaceData()  --> 
nsGenericDOMDataNode::SetText()  -->
nsDocument::ContentChanged()  -->
PresShell::ContentChanged() -->  create reflow coomand, enqueue it,... ,begin reflow

2) We can add a new function: DisableCache() in nsICaret, so that it can disable
current cache.

Comment 59

[leon.zhang]

first solution:

Index: base/src/nsCaret.h
===================================================================
RCS file: /cvsroot/mozilla/layout/base/src/nsCaret.h,v
retrieving revision 1.34
diff -u -r1.34 nsCaret.h
--- base/src/nsCaret.h  27 Feb 2003 23:08:23 -0000      1.34
+++ base/src/nsCaret.h  10 Mar 2003 07:12:00 -0000
@@ -105,6 +105,8 @@

     PRPackedBool          mCachedOffsetValid;   //whether the cached frame
offset is valid
     nsPoint               mCachedFrameOffset;   //cached frame offset
+
+    PRBool                mCaretBrowsingOn;   // whether caret in browser has
been turned on

     nsRect                mCaretRect;         // the last caret rect
     nsIFrame*             mLastCaretFrame;    // store the frame the caret was
last drawn in.
Index: base/src/nsCaret.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/base/src/nsCaret.cpp,v
retrieving revision 1.105
diff -u -r1.105 nsCaret.cpp
--- base/src/nsCaret.cpp        27 Feb 2003 23:08:18 -0000      1.105
+++ base/src/nsCaret.cpp        10 Mar 2003 07:12:05 -0000
@@ -63,6 +63,8 @@
 #include "nsWidgetsCID.h"     // for NS_LOOKANDFEEL_CID
 #include "nsBlockFrame.h"
 #include "nsISelectionController.h"
+#include "nsIPrefBranch.h"
+#include "nsIPrefService.h"

 #include "nsCaret.h"

@@ -162,6 +164,11 @@
       return rv;
   }

+  nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID));
+  NS_ENSURE_TRUE(prefBranch, NS_ERROR_FAILURE);
+  prefBranch->GetBoolPref("accessibility.browsewithcaret",
+                          &mCaretBrowsingOn);
+
 #ifdef IBMBIDI
   PRBool isRTL;
   mBidiKeyboard = do_GetService("@mozilla.org/widget/bidikeyboard;1");
@@ -1073,11 +1080,8 @@
     if (!mCachedOffsetValid)
     {
       mLastCaretFrame->GetPointFromOffset(presContext, mRendContext,
mLastContentOffset, &mCachedFrameOffset);
-      // XXX: The following line was commented out to disable the
-      //      offset caching code because it causes bad caret
-      //      placement. (bug 194774)
-      //
-      // mCachedOffsetValid = PR_TRUE;
+      if (!mCaretBrowsingOn)
+        mCachedOffsetValid = PR_TRUE;
     }

Comment 60

[leon.zhang]

comments#57 and comments#59 give 2 methods to ensure cache is compatibale with
accessibility.browsewithcaret. 
 

Comment 61

[Simon Fraser [no longer active]]

I don't really like either solution, though I think I prefer the first, if you
rename "DisableCache" to "InvalidateCachedOffset". What I don't like about this
patch is it pushes knowledge of caret internal data onto the presShell.

The second one isn't ideal because it assumes that browserwithcaret is the only
way to turn the caret on in a context where document changes are made outside of
the editor. I don't think that's a valid assumption.

Comment 62

[leon.zhang]

I will create a new patch based on simon's comments

Comment 63

[leon.zhang]

Attachment: turn on caching switch of caret

Comment 64

[leon.zhang]

Comment on attachment 116817 [details] [diff] [review]
turn on caching switch of caret

current patch can do
1) Same as last patch (attachment 116605 [details]), ensure the calculate correct cached
Caret pos after reflow(r=joe).

2) When content changed but frame and content offset still nonchanged,
invalidate cached caret pos, so that we can calculate a correct one again.

Comment 65

[kinmoz]

Comment on attachment 116817 [details] [diff] [review]
turn on caching switch of caret

I guess this looks ok. I don't really care for the fact that people that use
the caret have to know about caret implementation details and when to
invalidate it's cache.

So sfraser, does making the caret implement nsIDocumentObserver so that it gets
notifications directly seem like overkill?

I've been trying to think if we need to worry about other types of
notifications (style changes) that don't change the content, but could cause
the offset we cache to be invalid, but can't think of any cases off the top of
my head.

If sfraser is cool with this patch I'm inclined to give it an
r=[e-mail], but I'd like to hear from him regarding the
nsIDocumentObserver approach.

I'd just re-work the comment in nsICaret.h to say something like the following
so it matches the other comments:

+  /** InvalidateCachedOffset
+   *  Invalidate the cached frame offset so that it can be calculated again.
+   **/

Comment 66

[leon.zhang]

About next patch:

1) Main thought:
  Try to remove entry points of calling DrawCaret before Reflow, and keep the
point which is not related to Reflow.

2) Normally, process below should be ensured :
  content change  --> reflow  -> refresh cache(DrawCaret) -> use cache

3) Current points of calling DrawCaret:

nsAutoPlaceHolderBatch()
nsAutoRules() -> nsHTMLEditRules::BeforeEdit  -> ... -> DrawCaret    (*** delete it)
~nsAutoRules() -> nsHTMLEditRules::AfterEdit  -> ... -> DrawCaret    (*** delete it)


StCaretHider()  ... -> DrawCaret                                     (*** delete it)

nsTypedSelection::EndBatchChanges -> ... -> 
nsTypedSelection::NotifySelectionListeners ->
nsCaret::NotifySelectionChanged ->...-> DrawCaret                    (*** modify it)

EndReflowBatching                                                    (*** reflow
Process)

~StCaretHider()  ... -> DrawCaret                                     (***
delete it)

nsViewManager::EndUpdateViewBatch -> ... -> 
nsPresShell::DidRefreshRegion -> ... ->  DrawCaret                    (*** first
point of refreshing Cache after reflow )


-> nsEdiotor:: ScollSelectionIntoView     ...->                       (*** use
cache and need further optimization)
    StCaretHider() ... -> DrawCaret
    ~StCaretHider() ... -> DrawCaret
.......                                                                


4) Patch should do:
  (1) delete codes related to Drawcaret before reflow, ensure we have one stable
point to refresh cache when have reflow.

  (2) modify function nsCaret::NotifySelectionChanged to meet the need below:
      if reflow happen and we have gotten cache, use cache when Draw Caret;
      else if no reflow happen(e.g. move caret), we can call Draw Caret(also
will refresh cache),
      else if reflow happen and we have not gotten cache, exit(maybe a abnormal
status and we also should ensure one point to refresh).
      
  (3) In editor:
      when move caret, no reflow happen, nsCaret::NotifySelectionChanged can
draw caret correctly.
      when type chars and backspace, this lead to content change, reflow happen
and calculate new cached value.

  (4) In browser:
      When content changed(change DOM content using JS), this give rise to
reflow happen, view refresh and calculate new cached value.

Comment 67

[leon.zhang]

Current points of calling DrawCaret:

nsAutoPlaceHolderBatch()
 -- nsAutoRules() -> nsHTMLEditRules::BeforeEdit ->...-> DrawCaret 
|                                                    (*** delete it)
|  
 -- ~nsAutoRules() -> nsHTMLEditRules::AfterEdit ->...-> DrawCaret 
                                                      (*** delete it)

~nsAutoPlaceHolderBatch() ->
 --  nsEditor::EndPlaceHolderTransaction ->
|       nsEditor:: EndUpdateViewBatch()
|       --  StCaretHider()  ... -> DrawCaret          (*** delete it)
|      |
|      |    nsTypedSelection::EndBatchChanges -> ... -> 
|      |--  nsTypedSelection::NotifySelectionListeners ->
|      |    nsCaret::NotifySelectionChanged ->...-> DrawCaret 
|      |                                              (*** modify it) 
|      |--  EndReflowBatching                         (*** reflow Process ***)
|      |
|      |--  ~StCaretHider()  ... -> DrawCaret         (*** delete it)
|      |
|       --  nsViewManager::EndUpdateViewBatch -> ... -> 
|           nsPresShell::DidRefreshRegion -> ... ->  DrawCaret    
|                        (*** first point of refreshing Cache after reflow ***)
|
|
|--> nsEdiotor:: ScollSelectionIntoView ...->     (*** use cache **) 
       StCaretHider() ... -> DrawCaret            (***need further optimization) 
       ~StCaretHider() ... -> DrawCaret
       .......                                                                

Comment 68

[leon.zhang]

Attachment: patch(based on previous comments #66, #67)

Comment 69

[leon.zhang]

Attachment: patch(take style change into account)

Comment 70

[leon.zhang]

Attachment: patch(make DrawCaret more clearer)

Comment 71

[leon.zhang]

about current patch:
1) When content change or style change, we should draw caret after reflow, any 
   DrawCaret before reflow should be forbidden. In current patch, entry point of 
   draw caret in this condition is nsViewManager::EndUpdateViewBatch.

2) When move caret, nsCaret::NotifySelectionChanged will be called directly.
   we should allow this if no related reflow will happen or cache have ready.

3) We have 2 level mechanism to control the status of our cache:
   level 1: has reflow happen?  if happen, disable our cache.
   level 2: have frame and content offset changed? if anyone changed, disable
our cache.
   
   so when other entry point of calling DrawCaret(focus, blur), we can ensure
cache has been processed correctly by means of level 2.

   Other abnormal status, e.g. reflow happen, but our cache not ready, we still
keep orignal procedure, but cache's staus is still not ready.

4) Now we pay attention to two types of reflow which will give rise to drawcaret: 
   content change and style change.

Comment 72

[leon.zhang]

Attachment: new patch

Comment 73

[leon.zhang]

Attachment: cache value just for editor module

Comment 74

[leon.zhang]

Comment on attachment 117393 [details] [diff] [review]
cache value just for editor module

current patch is a new solution:
1) ensure draw caret after reflow
2) use cache only in editor module, not changed cany codes in nsIPresShell

Comment 75

[leon.zhang]

Attachment: a new concise patch

turn on/off cache are in the same level

Comment 76

[leon.zhang]

Comment on attachment 117676 [details] [diff] [review]
a new concise patch

test this patch using quantify based on trunk20030315, the time consumed by
DrawCaret decrease from about 12.5% to about 6.5%.

Comment 77

[leon.zhang]

Attachment: change name of function from SetOptimizeDrawCaretFlag() to SetOptimizeDrawCaret()

Maybe this name is more reasonable.

Comment 78

[kinmoz]

Comment on attachment 117714 [details] [diff] [review]
change name of function from SetOptimizeDrawCaretFlag() to SetOptimizeDrawCaret()

==== There's no need to use a PRPackedBool for a function:


+  NS_IMETHOD SetOptimizeDrawCaret(PRPackedBool aOptimzeDrawCaret) = 0;


==== The 'i' in "Optimze" is missing:


+    PRPackedBool	   mOptimzeDrawCaret;	 // flag for optimizing draw
caret


==== Should the PRPackedBools be next to each other so that the compiler can
optimize them together into the same byte? Not sure if the ordering matters.


-    PRPackedBool	   mCachedOffsetValid;	 //whether the cached frame
offset is valid
-    nsPoint		   mCachedFrameOffset;	 //cached frame offset
+    PRPackedBool	   mCachedOffsetValid;	 // whether the cached frame
offset is valid
+    nsPoint		   mCachedFrameOffset;	 // cached frame offset
+    PRPackedBool	   mOptimzeDrawCaret;	 // flag for optimizing draw
caret



==== This can probably be re-written to be simpler:


-    // if mCachedOffsetValid, apply cached FrameOffset, else compute it again
-    if (!mCachedOffsetValid) 
-    {
+    // if use cache and cache is ready, apply it, else refresh it
+    if (mOptimzeDrawCaret){
+      if (!mCachedOffsetValid) {
+	 mLastCaretFrame->GetPointFromOffset(presContext, mRendContext,
mLastContentOffset, &mCachedFrameOffset);
+	 mCachedOffsetValid = PR_TRUE;
+      } 
+    }
+    else {
       mLastCaretFrame->GetPointFromOffset(presContext, mRendContext,
mLastContentOffset, &mCachedFrameOffset);
-      // XXX: The following line was commented out to disable the
-      //      offset caching code because it causes bad caret
-      //      placement. (bug 194774)
-      //
-      // mCachedOffsetValid = PR_TRUE;



perhaps:



  if (!mOptimizeDrawCaret || !mCachedOffsetValid) {
    mLastCaretFrame->GetPointFromOffset(presContext, mRendContext,
mLastContentOffset, &mCachedFrameOffset);
    mCachedOffsetValid = mOptimizeDrawCaret;
  }



==== I'm a little nervous about removing the StCaretHider in
EndUpdateViewBatch() it's original purpose was so that the caret would
update/correctly position itself after the reflow ... we have to keep in mind
that 3rd parties could be pumping their own transactions through the editor and
using the Begin/EndUpdateView() methods ... bypassing your modifications in the
EndPlaceholder method .. not forcing the caret to redraw could leave it in the
wrong place.



-  StCaretHider caretHider(caret);
-	 
-  nsCOMPtr<nsISelection>selection;
-  nsresult selectionResult = GetSelection(getter_AddRefs(selection));
-  if (NS_SUCCEEDED(selectionResult) && selection) {
-    nsCOMPtr<nsISelectionPrivate>selPrivate(do_QueryInterface(selection));
-    selPrivate->EndBatchChanges();
-  }
-

Comment 79

[leon.zhang]

Attachment: new patch(according to kin's comments)

Comment 80

[leon.zhang]

Comment on attachment 117987 [details] [diff] [review]
new patch(according to kin's comments) 

kin: thx for your comments :)

according to your comments, I do below:
1) modify typo of previous patch :)

2) replace "PRPackedBool aOptimzeDrawCaret" with "PRBool aOptimzeDrawCaret" in
function
   NS_IMETHOD SetOptimizeDrawCaret(PRPackedBool aOptimzeDrawCaret) = 0;

3) reorder the sequence of member vars of nsCaret

4) simplify codes related DrawCaret

5) about "StCaretHider caretHider(caret);" in function:
nsEditor::EndUpdateViewBatch

   you will meet stack below which happens before reflow (or before calling
~AutoPlaceHolderBatch()) 
   if we keep original codes "StCaretHider caretHider(caret);" 

nsCaret::GetCaretRectAndInvert() line 1087
nsCaret::DrawCaret() line 994
nsCaret::StartBlinking() line 534
nsCaret::SetCaretVisible() line 236 + 8 bytes
StCaretHider::~StCaretHider() line 160
nsEditor::EndUpdateViewBatch() line 4408 + 23 bytes
nsEditor::InsertTextIntoTextNodeImpl() line 2509
nsEditor::InsertTextImpl() line 2406 + 34 bytes
nsWSRunObject::InsertText() line 450 + 46 bytes
nsHTMLEditRules::WillInsertText() line 1399 + 50 bytes
nsHTMLEditRules::WillDoAction() line 519 + 51 bytes
nsPlaintextEditor::InsertText() line 1005 + 56 bytes
nsPlaintextEditor::TypedText() line 574 + 26 bytes
nsHTMLEditor::TypedText() line 1419 + 17 bytes
nsHTMLEditor::HandleKeyPress() line 1397 + 34 bytes
nsTextEditorKeyListener::KeyPress() line 309   

   It means we have called drawCaret before reflow, caused a useless drawcaret
lead to perf issues.
   so I think we can move it to the position in current patch or we can delete
it completely 
   (BTW, original thought of deleting "StCaretHider caretHider(caret);"  is to
ensure the only entry point 
   of refreshing the cache: "mViewManager->EndUpdateViewBatch(updateFlag);" )

   Other points calling nsEditor::EndUpdateViewBatch should not be influnced by
moving or removing "StCaretHider".
   IMHO, we have enough DrawCaret points to draw caret correctly if no reflow
happen and we have modified those codes.
   I have test many, and not found erroe during drawcaret when we delete or
moving those codes :)

   btw, patches for bug 54153 have deleted those codes too.


-------------------------------------------------------------------------------
--------------------------------

Some testing data: 
  typed about 1000 chars in composer based on trunk 20030315 + solaris 8


Format of test data :
  test1 / test2
   *average*


Details:
		    orignal codes      current patch(move	 another
patch(remove
		 (without any patch)  stcaretHider before reflow)  
stcaretHider)
nsPresShell::	    15.09% / 15.33%	8.93% / 7.99%		     9.03% /
9.33%
SetCaretEnabled        *15.21%* 	   *8.46%*			*9.18%*

nsCaret::	    18.98% / 19.30%	13.46% / 12.67% 	       11.36%
/11.67%
SetCaretVisible        *19.14%* 	   *13.07%*			
*11.51%*

nsCaret::	    12.40% / 12.50%	6.05% / 6.61%		       6.48% /
6.64%
DrawCaret	       *12.48%* 	   *6.33%*			 
*6.56%*

nsTextFrame::	    10.52% / 10.60%	3.47% / 3.81%		       3.72% /
3.83%
GetPointFromOffset     *10.56%* 	   *3.64%*			  
*3.78%*

Comment 81

[leon.zhang]

paste test data again:
                 orignal codes      current patch        another patch
              (without any patch)  (move stcaretHider    (remove 
                                     before reflow)       stcaretHider)
nsPresShell::      15.09% / 15.33%     8.93% / 7.99%     9.03% / 9.33%
SetCaretEnabled        *15.21%*           *8.46%*           *9.18%*

nsCaret::          18.98% / 19.30%     13.46% / 12.67%   11.36% /11.67%
SetCaretVisible        *19.14%*           *13.07%*          *11.51%*

nsCaret::          12.40% / 12.50%     6.05% / 6.61%      6.48% / 6.64%
DrawCaret             *12.48%*            *6.33%*            *6.56%*

nsTextFrame::      10.52% / 10.60%     3.47% / 3.81%      3.72% / 3.83%
GetPointFromOffset    *10.56%*            *3.64%*            *3.78%*

Comment 82

[kinmoz]

Comment on attachment 117987 [details] [diff] [review]
new patch(according to kin's comments) 

I asked leon on IRC this same question, but thought I'd mention it here so
sfraser can see it when he  gets back from vacation.

Isn't the whole point of the current patch to simply avoid re-calculating caret
position more than once across the EndUpdateViewBatch() and
ScrollSelectionIntoView() call in EndPlaceHolderTransaction()? If so, can't the
same thing be accomplished by simply declaring an StCaretHider auto variable
before the EndUpdateViewBatch() call?

That should delay caret drawing (which actually calculates the caret position)
till after both of those calls are made.

Comment 83

[leon.zhang]

Attachment: test patch accroding to idea from comment#82

I test this patch according to idea from comment#82, and find that we still
call DrawCaret and time-consuming nsTextFrame::GetPointFromOffset times when we
type a char.

so I think it can not resolve perf of drawcaret completely.

Comment 84

[leon.zhang]

Comment on attachment 118426 [details] [diff] [review]
test patch accroding to idea from comment#82

applied this patch, drawcaret's caller mainly from:
1) nsViewManager::EndUpdateViewBatch --> ... --> nsViewManager::Refresh -> ...
--> PresShellViewEventListener::DidRefreshRegion

2) nsEditor::ScrollSelectionIntoView  ->  ... -> nsScrollPortView::ScrollTo ->
PresShellViewEventListener::ScrollPositionDidChange

3) nsEditor::EndPlaceHolderTransaction	 -> StCaretHider::~StCaretHider()

so cache still is necessary.

Comment 85

[kinmoz]

Comment on attachment 117987 [details] [diff] [review]
new patch(according to kin's comments) 

sr=[e-mail]

I guess I'm fine with this  patch for now ... there's still some cleaning up to
do in Begin/EndUpdateViewBatch(), for example we retrieven the a pointer to 
the PresShell twice in EndUpdateViewBatch(), but I already have a patch that
cleans both of those methods up for another bug.

By the way, my proposal above probably doesn't work because the presShell code
checks to see if the  caret is enabled/disabled rather then shown/visible.

Comment 86

[kinmoz]

Comment on attachment 117987 [details] [diff] [review]
new patch(according to kin's comments) 

I forgot to mention that before you checkin, make sure the order these are
initialized matches the order they are	declared in the class to avoid compiler
warnings on some of the platforms:

 , mCachedOffsetValid(PR_FALSE)
+, mOptimizeDrawCaret(PR_FALSE)

Comment 87

[leon.zhang]

kin: thx a lot for your sr :)

Comment 88

[leon.zhang]

before check in this patch, we should do:
1) modify the sequence of initilization of vars of nsCaret accroding to comments86
2) modify comments of GetOptimizeDrawCaret() in nsIcaret.h
from "+  /** SetOptimizeDrawCaretFlag" to "+  /** SetOptimizeDrawCaret"
becuase current function name is "SetOptimizeDrawCaret".

Comment 89

[leon.zhang]

Attachment: final patch

1) modify the sequence of initialization of vars in nsCaret according to kin's
suggestions in comments #86
2) modify the comments of function GetOptimizeDrawCaret() in nsIcaret.h to
match with current function name.

Comment 90

[leon.zhang]

Comment on attachment 118603 [details] [diff] [review]
final patch

transfer r=sfraser sr=kin to this patch

Comment 91

[Asa Dotzler [:asa]]

Comment on attachment 118603 [details] [diff] [review]
final patch

Please hold this until we open for 1.4beta. Thanks.

Comment 92

[leon.zhang]

patch checked in by [e-mail].

thx smfr, kin,jfrancis,henry.

Comment 93

[leon.zhang]

further optimization will be done in bug 194343 199412

Comment 94

[Boris Zbarsky [:bzbarsky]]

Could someone fix the spelling of aOptimizeDrawCaret in that last patch, please?

Comment 95

[leon.zhang]

Attachment: correct spelling error

thx bz

Comment 96

[Ere Maijala (slow)]

I filed bug 200545 (which will probably get duped anyway :) about caret not
being visible at all when replying to a message. Could it be related to this?

Comment 97

[leon.zhang]

Comment on attachment 119244 [details] [diff] [review]
correct spelling error

checked in