SetCaretEnabled() takes too long.

RESOLVED FIXED in Future

Status

()

Core
Selection
P2
critical
RESOLVED FIXED
18 years ago
14 years ago

People

(Reporter: Joe Francis, Assigned: Simon Fraser)

Tracking

(Blocks: 4 bugs, {helpwanted, perf})

Trunk
Future
helpwanted, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 12 obsolete attachments)

3.09 KB, patch
Details | Diff | Splinter Review
100.45 KB, image/jpeg
Details
5.48 KB, patch
Details | Diff | Splinter Review
181.58 KB, image/jpeg
Details
153.11 KB, image/jpeg
Details
1.83 KB, patch
Details | Diff | Splinter Review
3.71 KB, patch
Simon Fraser
: review+
Simon Fraser
: superreview+
Details | Diff | Splinter Review
2.13 KB, patch
Joe Francis
: review+
Details | Diff | Splinter Review
948 bytes, text/html
Details
7.53 KB, patch
Simon Fraser
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
7.58 KB, patch
Details | Diff | Splinter Review
1.94 KB, patch
Simon Fraser
: review+
Simon Fraser
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

18 years ago
Blocks: 28783
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Keywords: perf
Summary: PERF: SetCaretEnabled() takes too long. → SetCaretEnabled() takes too long.
(Assignee)

Updated

18 years ago
Target Milestone: --- → M17

Comment 1

17 years ago
m19
Target Milestone: M17 → M19

Comment 2

17 years ago
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

17 years ago
*SPAM*: Changing the QA contact of all open/resolved Selection bugs from 
elig@netscape.com to BlakeR1234@aol.com.  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...
QA Contact: elig → BlakeR1234

Comment 4

17 years ago
reviewed by Bijal and beppe, setting to nsbeta3+, p2
Keywords: nsbeta3
Priority: P3 → P2
Whiteboard: [nsbeta3+]{p:2]

Updated

17 years ago
Blocks: 53252

Updated

17 years ago
No longer blocks: 28783

Updated

17 years ago
Whiteboard: [nsbeta3+]{p:2] → [nsbeta3+][p:2]

Comment 5

17 years ago
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.
Priority: P2 → P3
Whiteboard: [nsbeta3+][p:2] → [nsbeta3+][p:2][pdtp3]
Target Milestone: M19 → Future

Comment 6

17 years ago
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.
Keywords: helpwanted
Whiteboard: [nsbeta3+][p:2][pdtp3] → [nsbeta3-][p:2][pdtp3]
(Reporter)

Updated

16 years ago
Blocks: 28783

Comment 7

16 years ago
Removing adequated PDT grafitti.
Whiteboard: [nsbeta3-][p:2][pdtp3]

Comment 8

16 years ago
changing selection qa to tpreston.
QA Contact: blaker → tpreston

Comment 9

16 years ago
removing myself from the cc list

Updated

15 years ago
Blocks: 188318

Comment 10

15 years ago
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
Severity: major → critical
Priority: P3 → P2

Comment 11

15 years ago
PresShell::SetCaretEnabled ---> nsCaret::SetCaretVisible

Comment 12

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
Created attachment 114088 [details]
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

15 years ago
Created attachment 114089 [details] [diff] [review]
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.
:)
(Assignee)

Comment 17

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

Comment 18

15 years ago
Created attachment 114160 [details]
test result of caching function: nsTextFrame::GetPointFromOffset

test result of caching function: nsTextFrame::GetPointFromOffset
Attachment #114088 - Attachment is obsolete: true
(Assignee)

Comment 19

15 years ago
Created attachment 114162 [details] [diff] [review]
test patch to cache the frame offset in the caret code

Comment 20

15 years ago
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

15 years ago
btw, nsCaret::ClearFrameRefs also has not been called.
(Assignee)

Comment 22

15 years ago
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

15 years ago
Created attachment 114284 [details]
Deatailed info about time consumed when calling nsCaret::DrawCaret

Comment 24

15 years ago
Created attachment 114289 [details]
detailed time info about function: nsTexFrame::GetPointFromOffset

Comment 25

15 years ago
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

15 years ago
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

15 years ago
Created attachment 114323 [details] [diff] [review]
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.

Updated

15 years ago
Attachment #114323 - Attachment description: test patch(cache in nxTextFrame, increase additional 12 bytes) → test patch(cache info in nsTextFrame, increase additional 12 bytes)
(Assignee)

Comment 28

15 years ago
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

15 years ago
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

15 years ago
Created attachment 114639 [details] [diff] [review]
a little modification to attachment 114162 [details] [diff] [review] (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

15 years ago
Created attachment 114743 [details] [diff] [review]
new version(according to suggestions of smfr)
Attachment #114639 - Attachment is obsolete: true

Updated

15 years ago
Attachment #114743 - Flags: superreview?(sfraser)
Attachment #114743 - Flags: review?(sfraser)

Comment 32

15 years ago
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

15 years ago
btw, testing data in above comments is produced by means of rational quantify.
(Assignee)

Updated

15 years ago
Attachment #114743 - Flags: superreview?(sfraser)
Attachment #114743 - Flags: superreview+
Attachment #114743 - Flags: review?(sfraser)
Attachment #114743 - Flags: review+

Comment 34

15 years ago
thx smfr

Comment 35

15 years ago
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. .."

Updated

15 years ago
Blocks: 194343

Comment 36

15 years ago
file a new bug (http://bugzilla.mozilla.org/show_bug.cgi?id=194343) for
following optimization
No longer blocks: 194343

Updated

15 years ago
Blocks: 194343

Comment 37

15 years ago
checked in by henry.jia.

thx smfr, henry.

further optimization will be done in bug 194343.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

15 years ago
No longer blocks: 194343

Updated

15 years ago
Blocks: 194343
(Assignee)

Comment 38

15 years ago
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.
(Assignee)

Comment 39

15 years ago
This probably caused bug 194774.

Comment 40

15 years ago
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

15 years ago
Reopening this bug. I checked in the patch for bug 194774 for leon which
disables the caching code landed for this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 42

15 years ago
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

15 years ago
Created attachment 115631 [details] [diff] [review]
test patch for regression
(Assignee)

Comment 44

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

Comment 45

15 years ago
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?
Attachment #115631 - Attachment is obsolete: true

Comment 46

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
Created attachment 115952 [details] [diff] [review]
new patch

Comment 50

15 years ago
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.
(Assignee)

Comment 51

15 years ago
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.

Updated

15 years ago
Attachment #115952 - Flags: superreview?(sfraser)
Attachment #115952 - Flags: review?(kin)
(Assignee)

Comment 52

15 years ago
Comment on attachment 115952 [details] [diff] [review]
new patch

Joe needs to look at this.
Attachment #115952 - Flags: review?(kin) → review?(jfrancis)

Comment 53

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

Updated

15 years ago
Depends on: 195898
(Reporter)

Updated

15 years ago
Attachment #115952 - Flags: review?(jfrancis) → review+
(Assignee)

Comment 54

15 years ago
Created attachment 116605 [details]
Testcase for the patch

Comment 55

15 years ago
Comment on attachment 116605 [details]
Testcase for the patch

current patch dose not work for this testcase :(

Comment 56

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
comments#57 and comments#59 give 2 methods to ensure cache is compatibale with
accessibility.browsewithcaret. 
 
(Assignee)

Comment 61

15 years ago
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

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

Comment 63

15 years ago
Created attachment 116817 [details] [diff] [review]
turn on caching switch of caret

Updated

15 years ago
Attachment #116817 - Flags: superreview?(sfraser)
Attachment #116817 - Flags: review?(kin)

Comment 64

15 years ago
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

15 years ago
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=kin@netscape.com, 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

15 years ago
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

15 years ago
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

15 years ago
Created attachment 116977 [details] [diff] [review]
patch(based on previous comments #66, #67)

Comment 69

15 years ago
Created attachment 116982 [details] [diff] [review]
patch(take style change into account)
Attachment #116977 - Attachment is obsolete: true

Updated

15 years ago
Flags: blocking1.4a?

Comment 70

15 years ago
Created attachment 117083 [details] [diff] [review]
patch(make DrawCaret more clearer)
Attachment #116982 - Attachment is obsolete: true

Comment 71

15 years ago
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

15 years ago
Created attachment 117273 [details] [diff] [review]
new patch
Attachment #117083 - Attachment is obsolete: true

Updated

15 years ago
Attachment #117273 - Flags: superreview?(sfraser)
Attachment #117273 - Flags: review?(kin)

Updated

15 years ago
Attachment #117273 - Attachment is obsolete: true
Attachment #117273 - Flags: superreview?(sfraser)
Attachment #117273 - Flags: review?(kin)

Comment 73

15 years ago
Created attachment 117393 [details] [diff] [review]
cache value just for editor module
Attachment #116817 - Attachment is obsolete: true

Comment 74

15 years ago
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
Attachment #117393 - Flags: superreview?(sfraser)
Attachment #117393 - Flags: review?(kin)

Updated

15 years ago
Attachment #116817 - Flags: superreview?(sfraser)
Attachment #116817 - Flags: review?(kin)

Updated

15 years ago
Attachment #117393 - Attachment is obsolete: true
Attachment #117393 - Flags: superreview?(sfraser)
Attachment #117393 - Flags: review?(kin)

Comment 75

15 years ago
Created attachment 117676 [details] [diff] [review]
a new concise patch

turn on/off cache are in the same level

Updated

15 years ago
Attachment #117676 - Flags: superreview?(sfraser)
Attachment #117676 - Flags: review?(kin)

Comment 76

15 years ago
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

15 years ago
Created attachment 117714 [details] [diff] [review]
change name of function from SetOptimizeDrawCaretFlag() to SetOptimizeDrawCaret()

Maybe this name is more reasonable.

Updated

15 years ago
Attachment #117714 - Flags: superreview?(sfraser)
Attachment #117714 - Flags: review?(kin)

Updated

15 years ago
Attachment #117676 - Flags: superreview?(sfraser) → superreview?(kin)

Updated

15 years ago
Attachment #117714 - Flags: superreview?(sfraser) → superreview?(kin)

Comment 78

15 years ago
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();
-  }
-

Updated

15 years ago
Attachment #117676 - Attachment is obsolete: true
Attachment #117676 - Flags: superreview?(kin)
Attachment #117676 - Flags: review?(kin)

Updated

15 years ago
Attachment #117714 - Attachment is obsolete: true
Attachment #117714 - Flags: superreview?(kin)
Attachment #117714 - Flags: review?(kin)

Comment 79

15 years ago
Created attachment 117987 [details] [diff] [review]
new patch(according to kin's comments)

Comment 80

15 years ago
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%*
Attachment #117987 - Flags: superreview?(kin)
Attachment #117987 - Flags: review?(kin)

Comment 81

15 years ago
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%*

Updated

15 years ago
Flags: blocking1.4a? → blocking1.4a-

Comment 82

15 years ago
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

15 years ago
Created attachment 118426 [details] [diff] [review]
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

15 years ago
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

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

sr=kin@netscape.com

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.
Attachment #117987 - Flags: superreview?(kin) → superreview+

Comment 86

15 years ago
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

15 years ago
kin: thx a lot for your sr :)
(Assignee)

Updated

15 years ago
Attachment #117987 - Flags: review?(kin) → review+

Comment 88

15 years ago
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".

Updated

15 years ago
Attachment #117987 - Flags: approval1.4a?

Updated

15 years ago
Attachment #117987 - Flags: approval1.4a?

Updated

15 years ago
Attachment #118426 - Attachment is obsolete: true

Comment 89

15 years ago
Created attachment 118603 [details] [diff] [review]
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

15 years ago
Comment on attachment 118603 [details] [diff] [review]
final patch

transfer r=sfraser sr=kin to this patch
Attachment #118603 - Flags: approval1.4a?

Comment 91

15 years ago
Comment on attachment 118603 [details] [diff] [review]
final patch

Please hold this until we open for 1.4beta. Thanks.
Attachment #118603 - Flags: approval1.4a? → approval1.4a-

Updated

15 years ago
Blocks: 199412

Comment 92

15 years ago
patch checked in by Henry.Jia@sun.com.

thx smfr, kin,jfrancis,henry.
Flags: blocking1.4a-

Comment 93

15 years ago
further optimization will be done in bug 194343 199412
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Blocks: 195798
Could someone fix the spelling of aOptimizeDrawCaret in that last patch, please?

Comment 95

15 years ago
Created attachment 119244 [details] [diff] [review]
correct spelling error

thx bz

Updated

15 years ago
Attachment #119244 - Flags: superreview?(sfraser)
Attachment #119244 - Flags: review?(sfraser)
(Assignee)

Updated

15 years ago
Attachment #119244 - Flags: superreview?(sfraser)
Attachment #119244 - Flags: superreview+
Attachment #119244 - Flags: review?(sfraser)
Attachment #119244 - Flags: review+

Comment 96

15 years ago
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

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

checked in

Updated

14 years ago
Blocks: 207936
(Assignee)

Updated

13 years ago
Attachment #115952 - Flags: superreview?(sfraser_bugs)
You need to log in before you can comment on or make changes to this bug.