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.

take it

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.

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.

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.

test result nsPreShell:: nsCaret:: nsCaret:: SetCaretEnabled SetCaretVisible DrawCaret 7.92% 7.91% 4.02% applied news patch

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

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

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

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

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 )

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

Comment on attachment 116506 [details] [diff] [review] new patch when typing quickly, It seems that the carte always lose behind the last char.

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.

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.

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%*

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.

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.

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%

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.

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

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

Comment on attachment 121174 [details] [diff] [review] proposed fix This patch removed redudent calling SetCaretVisible and decreased time consumed in timer thread.

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().

kin, simon, can you give some suggestions?

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

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

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

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.

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

patches for caret turd have been pasted into bug 205544.

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 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.

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)

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.

Caret turds are Mac-only: bug 204493

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.

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)".

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().

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.

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

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 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 on attachment 122300 [details] [diff] [review] Patch to get rid of stCaretHider in nsEditor.cpp this can reslove some cases related caret turd.

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.

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

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

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

patch approved. checked in? or waiting on something?

(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.

patch needs new owner