194343 - optimize process of caret

Status: NEW

(Core :: DOM: Selection, defect)

Description

[leon.zhang]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0

Caret process paly an important role in the perfomance of composer, it is
helpful to optimize the process of caret.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[leon.zhang]

take it

Comment 2

[leon.zhang]

A test based on last patch fro bug 35296:

  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 from calling 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. .."

Although time consumed by nsCaret::SetCaretVisible decreased, time consumed by
nsCaret::nsCaret::NotifySelectionChanged increased.

The latter function will call StartBlinking and StopBlinking which will call
DrawCaret, so the time consumed by DrawCaret changed a bit or almost be stable.

Comment 3

[leon.zhang]

follow previous comments:

Since nsCaret::NotifySelectionChanged consumed more time than before, we can
optimize it like below :

replace codes below:
  if (mVisible)
  {
    // Stop the caret from blinking in its previous location.
    StopBlinking();

    // Start the caret blinking in the new location.
    StartBlinking();
  }
with following codes:
  if (mVisible)
  {
    if (mDrawn)  {   // erase the caret if necessary 
      DrawCaret();
    }

    DrawCaret();
  }

Timer process is time-consuming, so we can remove those unnecessary codes from
orignal functions: StopBlinking/StartBlinking.

After that, time time consumed by nsCaret::NotifySelectionChanged decrease from
about 1.46% to 0.11% of the whole application.

At the same time, not find explict change in usability.

Comment 4

[leon.zhang]

Attachment: test patch 1 for comments #3

Comment 5

[leon.zhang]

Attachment: test patch2

Comment 6

[leon.zhang]

explaination for the second patch:

1) Because The caret did not display till composer window appear, So call
StartBlinking is useless

2) When user type a char, event( PresShellViewEventListener::WillRefreshRegion)
has been activiate and calls PresShellViewEventListener::HideCaret, so it is not
necessary to call PresShellViewEventListener::HideCaret again during process
event: PresShellViewEventListener::ScrollPositionWillChange.

Comment 7

[leon.zhang]

test result

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

7.92%                     7.91%                   4.02%       applied news patch

Comment 8

[leon.zhang]

After test comments #3, that action is not helpful to perf.

Comment 9

[leon.zhang]

Attachment: Orginal cpu usage(after patches for bug35296)

Comment 10

[leon.zhang]

Attachment: after curent patch

Comment 11

[leon.zhang]

env: trunk20030305 + last patch for 35296 + current patch +  windows2000
with the larhest width of c window of composer

Comment 12

[leon.zhang]

comment #8 should be disabled
(when test patch, I forget to modify codes related HiderCaret() and
RestoreCaretVisibility() in nsPresShell.cpp)

Comment 13

[leon.zhang]

Comment on attachment 115372 [details] [diff] [review]
test patch2

if we delete two nscareyHider and HideCaret/RestoreCaretVisibility, we cannot
compute our caret pos to cache.

if BACKSPACE, lead to regression, although thi spatch is helpful to perf

Comment 14

[leon.zhang]

StCaretHider caretHider(caret); (in nsEditor.cpp) should not be deleted, because
it can ensure:
1) calculated the corretc cached caret pos after reflow
2) the same entry point to get the correct cached caret pos.
   ( nsViewManager::EnableRefresh -> 
     nsViewManager::Composite ...>  
     PresShellViewEventListener::DidRefreshRegion )

Comment 15

[leon.zhang]

I will give a new version of patch according to previous comments

Comment 16

[leon.zhang]

Attachment: new patch

Comment 17

[leon.zhang]

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

when typing quickly, It seems  that the carte always lose behind the last char.

Comment 18

[leon.zhang]

Attachment: patch 1.0

explaination to current patch:
1) in file: nsCaret.cpp
  nsCaret::Init() 
   mVisible always is false when it been called firstly, so remove this useless
codes or comment them out.

  see codes in nsPresShell.cpp:
   #ifdef SHOW_CARET
  // make the caret
  nsresult  err = NS_NewCaret(getter_AddRefs(mCaret));
  if (NS_SUCCEEDED(err))
  {
    mCaret->Init(this);
  }

  //SetCaretEnabled(PR_TRUE);			// make it show in browser
windows
#endif	


2) in file nsSelection.cpp
   nsTypedSelection::ScrollIntoView been called after Caret have been drawn
(from PresShellViewEventListener::DidRefreshRegion()), so we should not call it
again.
   
3) in file nsPresShell.cpp
   deleting DrawCaret pocess in ScrollPositionDid/WillChange is the same reason
with above.

Comment 19

[leon.zhang]

Tested the patch using quantify, the time consumed in related functions have
been decreased, including:
nsCaret::SetCaretVisible,nsPresShell::SetCaretEnabled() and other functions
related to timer.

Comment 20

[leon.zhang]

After patch for bug 35296 have been checked in, we have test data below(part of
 bug 35296's comments#81)

                      orignal codes      after patch for bug35296

nsPresShell::        15.09% / 15.33%       8.93% / 7.99%   
SetCaretEnabled          *15.21%*             *8.46%*      

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

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

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

Comment 21

[leon.zhang]

According to testing data above, we will find that time consumed by DrawCaret
have decreased explictly(from 12% to 6%), but nsCaret::SetCaretVisible still
consumed abnormal cpu time(13%).

Most of the time consumed by nsCaret::SetCaretVisible are shared by function:
nsCaret::StartBlinking() and nsCaret::StopBlinking(). The total time consumed by
those two functions are 15% of whole app:  
whole time = nsCaret::SetCaretVisible(%13)+ nsCaret::NotifySelectionChanged(2%)  

Subtract time consumed by DrawCaret(6%) from whole time, the time consumed by
other functions related to timer about 9%.

Main timer related functions are: nsCaret::PrimeTimer() and
nsCaret::KillTimer(). Those fucntions will call TimerThread::AddTimer() and
TimerThread::RemoveTimer() at last.

In sol8 platform, most of time consumed by those two functions of TimerThread
focus on: PR_Unlock
PR_Unlock -> pt_PostNotifies -> pthread_Cond_signal.

Up to now, I do not think there exist perf in PR_Unlock.
But we still can improve perf here: decrease times of calling functions which
will lead to calling related Timer ( nsCaret::StartBlinking() and
nsCaret::StopBlinking(). 

In a word, we still should can remove redundant SetCaretVisible.

Comment 22

[leon.zhang]

according to analysis from comment #11 of bug 190244:
http://bugzilla.mozilla.org/show_bug.cgi?id=190244#c11 , we will find:
when nsViewManager::Refresh() is be called, SetCaretVisible will be activiated.

But, at least half of the times of calling nsViewManager::Refresh() are related
to refreshing horizontal scrollbar, which also activiate
nsCraet::SetCaretVisible() unnecessarily.

Comment 23

[leon.zhang]

move some testing data (for current trunk) from bug 199412 here:

nsPresShell::SetCaretEnabled        5.92%
nsCaret::SetCaretVisible            10.80%
nsCaret::DrawCaret                  6.25%
nsTextFrame::GetPointFromOffset     2.23%

Comment 24

[leon.zhang]

SetCaretVisible still consumed much cpu time, we should optimize it.
I think we can decrease times of calling it.
comments and patches in bug 54155 may be helpful to this bug.

Comment 25

[leon.zhang]

typo: comments and patches in bug 54153 may be helpful to this bug.

Comment 26

[leon.zhang]

Attachment: proposed fix

Removed codes like "StCaretHider" because:
if reflow happen, refresh view and scroll will happen sequentially and caret
have been drawn times correctly.

Comment 27

[leon.zhang]

Comment on attachment 121174 [details] [diff] [review]
proposed fix

This patch removed redudent calling SetCaretVisible and decreased time consumed
in timer thread.

Comment 28

[leon.zhang]

Attachment: proposed fix(implenation 1)

Comment 29

[leon.zhang]

Comment on attachment 121399 [details] [diff] [review]
proposed fix(implenation 1)

removed most of the unnessary SetCaretVisable resulted from refreshing view and
scroll, just keep calling from NotifySelectionChanged().

Comment 30

[leon.zhang]

Attachment: same thought, another implemenation

kin, simon,
can you give some suggestions?

Comment 31

[leon.zhang]

Comment on attachment 121399 [details] [diff] [review]
proposed fix(implenation 1)

After tested patch using quantify:
  whole app consumed cpu cycles decrease from  1,485,011,667 to  1,308,166,101,
and decrease about 11.9%.
  nsHTMLEditor::TypedText consumed cpu cycles decrease from 1,107,436,300  to 
1,020,110,008, and decrease about 7.8%.
  PR_Unlock  consumed cpu cycles decrease from 85,372,928  to  42,436,517, and
decrease about 50%.

Testing hardware platform:

Testing steps:
1) apply current patch to file nsHTMLEditor.cpp

  Index: nsHTMLEditor.cpp
===================================================================
RCS file: /cvsroot/mozilla/editor/libeditor/html/nsHTMLEditor.cpp,v
retrieving revision 1.470
diff -u -r1.470 nsHTMLEditor.cpp
--- nsHTMLEditor.cpp	17 Apr 2003 05:42:31 -0000	1.470
+++ nsHTMLEditor.cpp	23 Apr 2003 02:23:44 -0000
@@ -1435,9 +1435,16 @@
    to TypedText() to determine what action to take, but without passing
    an event.
    */
+int count_char = 0;
 NS_IMETHODIMP nsHTMLEditor::TypedText(const nsAString& aString,
				       PRInt32 aAction)
 {
+ count_char ++ ;
+ if (count_char == 1000)
+  printf("10000 chars \n");
+
+ if (count_char > 1000)
+   return NS_OK;
   nsAutoPlaceHolderBatch batch(this, gTypingTxnName);


2) apply current patch
3) run-mozilla.sh -g -d gdb mozilla-bin.quantify
4) when mozilla startup, type a char 'w' so that mozilla will load an unfound
library, erro message will appear: 
    quantify (par): Bad parameters : dlopen("libesd.so", 1) file (arg #1) not
found.	
5) start test
6) type chars till message below appear:
    "printf("10000 chars \n");"
7) collect testing data

Comment 32

[leon.zhang]

Testing hardware platform:
uname -a
  SunOS 5.8 ??? sun4u sparc SUNW,Sun-Blade-1000

2 CPU, 900MHZ

Comment 33

[leon.zhang]

Attachment: wrong attachment

Comment 34

[leon.zhang]

Attachment: clean codes

Comment 35

[Jo Hermans]

See also bug 101333 (caret still blinks while another app has focus), but that's
Mac-only.

Comment 36

[kinmoz]

Attachment: Patch to get rid of stCaretHider in nsEditor.cpp

Leon asked me to add some comments about removing the use of stCaretHider.
Right  now on the TRUNK, I believe that we are only using the caret hider in 2
places.

The first place is in the editor in EndUpdateViewBatch(). sfraser this is the
change mjudge, you and I were discussing a couple of weeks ago on one of the
branches we are working on.

I believe this instance should be replaced with calls that show and hide the
caret in BeginUpdateViewBatch() and EndUpdateViewBatch(), this will prevent any
call to ClearFrameRefs() during one of the edit operations from throwing the
caret show state out of sync. Also, since the caret is shared amongst *several*
selections on the page, we also need to make sure that we only show/hide the
caret if the current caret selection *is* the same selection the editor is
using for it's operations.

I've attached a patch that does the above. One problem I did notice, at least
on our branch, is that a call to |caret->SetCaretVisible(PR_TRUE)| while the
selection is uncollapsed, doesn't draw the caret, which is correct, but it
still fires off the draw timer.

The 2nd instance of the stCaretHider is in ScrollSelectionIntoView() in
nsSelection.cpp. I don't think that instance is necessary anymore because we
added code a while back to the presShell, to automatically turn off the caret
before scrolling and turn it back on afterwards via a notification from the
ViewManager. I also commented out this same instance in my patch for bug 54153
(Animated image causes textbox insertion point to flicker) when trying to
reduce the number of times we draw/hide the caret.

Comment 37

[leon.zhang]

Attachment: remove SetCaretHider in nsSelection.cpp

Comment 38

[Simon Fraser [no longer active]]

Before that last patch goes in, we should look at the cause of the caret turds
while arrow-scrolling.

Comment 39

[leon.zhang]

patches for caret turd have been pasted into bug 205544.

Comment 40

[leon.zhang]

Comment on attachment 122300 [details] [diff] [review]
Patch to get rid of stCaretHider in nsEditor.cpp

+    if (mDidHideCaret)
+    {
+      mDidHideCaret = PR_FALSE;
+
+      if (presShell)
+      {
+	 nsCOMPtr<nsICaret> caret;
+
+	 presShell->GetCaret(getter_AddRefs(caret));
+
+	 if (caret)
+	   caret->SetCaretVisible(PR_TRUE);

we can jusge whether the caret is visble, if so that we should not draw caret
again

Comment 41

[leon.zhang]

Comment on attachment 122300 [details] [diff] [review]
Patch to get rid of stCaretHider in nsEditor.cpp

I think in most case the caret is visible when run here.

Comment 42

[leon.zhang]

I pasted a patch (http://bugzilla.mozilla.org/attachment.cgi?id=124337&action=view) 
which intergrated the patches for bug 194343, bug 204434 and bug 204455.
This patch, and  give testing data in comments:
http://bugzilla.mozilla.org/show_bug.cgi?id=188318#c49

it can :
1) remove or skip unnessary caret process(including patches for 194343)
2) turn off caret before editing process (including kin's patch for 194343)
3) enhance cache for caret(including patch for 204434)

Comment 43

[kinmoz]

sfraser, you mentioned caret turds in comment 38 above ... were you arrowing
around in a text widget? Did the turds only happen when the view scrolled? Just
curious, because I did some poking around and found out that the text widgets
don't register a ScrollPositionListener on their scrolling views, which is the
mechanism the presShell uses to tell when it needs to hide/show the caret during
scrolling.

Comment 44

[Simon Fraser [no longer active]]

Caret turds are Mac-only: bug 204493

Comment 45

[kinmoz]

Comment on attachment 122356 [details] [diff] [review]
remove SetCaretHider in nsSelection.cpp

Before removing this CaretHider in the selection scrolling code, we need to
make sure that we don't get caret cruft when scrolling text widgets. If I
remember correctly they aren't ScrollPosition listeners like the
document/presShell that contains them is, so I don't think it gets the caret
automatically turned off during scrolling.

Replying to leon's comment 40 ...

> we can jusge whether the caret is visble, if so that we should not draw caret
> again

If you're implying that we should make that decision in EndUpdateViewBatch(),
we'd have to duplicate the decision code in nsCaret::MustDrawCaret() which is
called anyways ... the problem is we shouldn't be firing the timer off in
nsCaret::PrimeTimer() if MustDrawCaret() returns PR_FALSE.

Comment 46

[leon.zhang]

nsCaret::GetCaretVisible() is judging "mVisible" , and nsCaret::MustDrawCaret()
can judge "mDrawn".

So, I think: we should judge whether the caret is visible in
"EndUpdateViewBatch", if so, we should not call "caret->SetCaretVisible(PR_TRUE)".

Comment 47

[kinmoz]

The mVisible field controls whether or not the caret is enabled and can be drawn
if it needed to.

MustDrawCaret() does a little more than just look at mDrawn, it also makes
decisions based on paint suppression, and whether to actualy draw the caret
during a non-collapsed selection (which is controlled by the look and feel of
the platform).

My patch calls caret->SetCaretVisible(PR_FALSE) in BeginUpdateViewBatch() to
disable the caret to avoid the problem where mDrawn is thrown out of sync
because the frame the caret is attached to is destroyed. By the time we get to
EndUpdateViewBatch(), we know the caret is disabled and not drawn, so calling
caret->SetCaretVisible(PR_TRUE) is supposed to re-enable the caret, and because
that also triggers a call to MustDrawCaret() the caret already figures out
whether or not it should be drawing or not ... so there really shouldn't be a
need to duplicate all of the MustDrawCaret() logic in EndUpdateViewBatch().

Comment 48

[leon.zhang]

1) It is correct to turn off caret in "BeginUpdateViewBatch", becuase of caret
turd issues. This can "avoid the problem where mDrawn is thrown out of sync
because the frame the caret is attached to is destroyed"

2) In normal process when user type a char, calling
"caret->SetCaretVisible(PR_TRUE)" directly in "EndUpdateViewBatch" of current
patch is redundant:
     "mViewManager->EndUpdateViewBatch(updateFlag);" will lead to visible caret
at fisrt after reflow; "selPrivate->EndBatchChanges();" will call
nsCaret::NotifySelectionChanged() and drawcaret second time.
     If we call "caret->SetCaretVisible(PR_TRUE)" here, this is unnecessary in
this case.
   To ensure the caret is visible and dose not import extra process, I give
previous comments.

3) In fact, in previous three process, MustDrawCaret() always return false, and
draw caret each time.

Comment 49

[leon.zhang]

sorry, 3) of above comments should be:
3) In fact, in previous three process, MustDrawCaret() always return true, and
draw caret each time.

Comment 50

[kinmoz]

Hmmm, it looks like the presShell's Will/DidRefreshRegion() mechanism is
interfering here. When DidRefreshRegion() is triggered during the 
mViewManager->EndUpdateViewBatch() call, in EndUpdateViewBatch(), it forces the
caret on ... we'll have to change that to respect the caret's visibility flag
setting when WillRefreshRegion() is called.

Comment 51

[leon.zhang]

Comment on attachment 122300 [details] [diff] [review]
Patch to get rid of stCaretHider in nsEditor.cpp

kin, then what can we do now?  I think this patch is helpful to some caret 
turd bugs, although it can also  import perf issues.

maybe we can resolve perf in another patch  of this bug?

Comment 52

[leon.zhang]

Comment on attachment 122300 [details] [diff] [review]
Patch to get rid of stCaretHider in nsEditor.cpp

this can reslove some cases related caret turd.

Comment 53

[kinmoz]

Comment on attachment 122356 [details] [diff] [review]
remove SetCaretHider in nsSelection.cpp

Leon, the only thing holding up r/sr=kin on this patch is your feedback on my
concern in comment 45. Removal of this caret hider needs to be tested on the
various platforms to make sure we don't leave caret cruft around when scrolling
while arrowing around or using the scrollbar controls in text widgets.

Comment 54

[leon.zhang]

*** Bug 204493 has been marked as a duplicate of this bug. ***

Comment 55

[leon.zhang]

*** Bug 205544 has been marked as a duplicate of this bug. ***

Comment 56

[Prognathous]

Adding Bug 265061 to the depends list: ("Switching input languages should skip
caret blinks and display the new caret immediately")

Prog.

Comment 57

[Wayne Mery (:wsmwk)]

patch approved.

checked in?
or waiting on something?

Comment 58

[Prognathous]

(In reply to comment #57)
> checked in?

Probably not. Leon checked in two more patches to nsEditor.cpp, but they were
for the 1.4 branch and did not include his changes in attachment #122300 [details] [diff] [review].

Bonsai is your friend: http://bonsai.mozilla.org/cvsqueryform.cgi?cvsroot

Prog.

Comment 59

[Wayne Mery (:wsmwk)]

patch needs new owner

Comment 60

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.