199412 - optimize nsTypedSelection::ScrollIntoView()

Status: RESOLVED FIXED

(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

we have cached offset of caret in text frame in bug 35296,and scrollintoview
always called after we have refreshed cached value, 
so we can use it cached value when: 
nsTypedSelection::ScrollIntoView() ->
nsTypedSelection::GetSelectionRegionRectAndScrollableView() ->
nsTypedSelection::GetPointFromOffset() -> nsTextFrame::GetPointFromOffset()

through this we can improve perf of editor about 2%.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[leon.zhang]

assigned to me

Comment 2

[leon.zhang]

After patche for bug 35206 has been committed, I will give a patch based on this

Comment 3

[leon.zhang]

typo:
After patch for bug 35296 has been committed, I will give a patch based on that bug.

Comment 4

[leon.zhang]

Attachment: patch 1.0

Comment 5

[Simon Fraser [no longer active]]

I don't think it's appropriate to be using the caret as a cache storage object.
It would make much more sense for the selection to have the cache, with some
method that wraps GetPointFromOffset(), returning a cached value when possible.

Comment 6

[leon.zhang]

Attachment: test patch

Comment 7

[leon.zhang]

Attachment: move cache from caret to selection

Comment 8

[Simon Fraser [no longer active]]

Comment on attachment 119955 [details] [diff] [review]
move cache from caret to selection

>Index: content/base/public/nsISelection.idl
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsISelection.idl,v

>+
>+    /**
>+     * Returns cache for nsIFrame::GetPointFromOffset.
>+     */
>+    [noscript] void GetOptimizeDrawCaret(out PRBool aOptimzeDrawCaret, out nsIFrame aFrame, out PRInt32 inOffset, out nsPoint aPoint);
>+
>+    /**
>+     * Sets cache for nsIFrame::GetPointFromOffset.
>+     */
>+    [noscript] void SetOptimizeDrawCaret(in PRBool aOptimzeDrawCaret, in nsIFrame aFrame, in PRInt32 inOffset, in nsPoint aPoint);
> };

I don't think we want to mess with nsISelection, which is a public interface.
nsISelectionPrivate would be a better API to put these on. Also, it's not
'OptimizeDrawCaret' draw caret any more. It's more like
GetCachedOffsetForFrame(). There should be no 'SetCachedOffset'.


>Index: content/base/src/nsSelection.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsSelection.cpp,v
>retrieving revision 3.142
>diff -u -r3.142 nsSelection.cpp
>--- content/base/src/nsSelection.cpp	12 Mar 2003 03:21:49 -0000	3.142
>+++ content/base/src/nsSelection.cpp	9 Apr 2003 10:23:17 -0000
>@@ -218,6 +218,9 @@
>   NS_IMETHOD    GetInterlinePosition(PRBool *aInterlinePosition);
>   NS_IMETHOD    SetInterlinePosition(PRBool aInterlinePosition);
>   NS_IMETHOD    SelectionLanguageChange(PRBool aLangRTL);
>+  NS_IMETHOD    GetOptimizeDrawCaret(PRBool *aOptimzeDrawCaret, nsIFrame **aFrame, PRInt32 *inOffset, nsPoint ** aPoint);
>+  NS_IMETHOD    SetOptimizeDrawCaret(PRBool aOptimzeDrawCaret, nsIFrame *aFrame, PRInt32 inOffset, nsPoint * aPoint);
>+
> 
> /*END nsISelection interface implementations*/
> 
>@@ -311,6 +314,12 @@
>   PRBool mTrueDirection;
>   nsCOMPtr<nsIEventQueue> mEventQueue;
>   PRBool mScrollEventPosted;
>+
>+  PRPackedBool mOptimizeDrawCaret;   // flag for optimizing draw caret
>+  PRPackedBool mCachedOffsetValid;   // whether the cached frame offset is valid
>+  nsPoint      mCachedFrameOffset;   // cached frame offset
>+  nsIFrame*    mLastCaretFrame;      // store the frame the caret was last drawn in.
>+  PRInt32      mLastContentOffset;
> };

is mOptimizeDrawCaret necessary? I'm also worried that you're adding about 20
bytes to each selection object. Maybe put these into a struct that is only
allocated if used?

>+NS_IMETHODIMP    
>+nsTypedSelection::GetOptimizeDrawCaret(PRBool *aOptimzeDrawCaret, nsIFrame **aFrame, PRInt32 *inOffset, nsPoint ** aPoint)
>+{

This method should be called GetCachedOffsetForFrame(). It should return the
cached offset if it's valid, otherwise call GetPointFromOffset() on the frame,
then cache the result.

>+NS_IMETHODIMP
>+nsTypedSelection::SetOptimizeDrawCaret(PRBool aOptimzeDrawCaret, nsIFrame *aFrame, PRInt32 inOffset, nsPoint * aPoint)
>+{
>+  mOptimizeDrawCaret = aOptimzeDrawCaret;
>+  mLastCaretFrame = aFrame;
>+  mLastContentOffset = inOffset;
>+  mCachedFrameOffset.x = aPoint->x;
>+  mCachedFrameOffset.y = aPoint->y;
>+  return NS_OK;
>+}

This method is not necessary.

> nsresult
> nsTypedSelection::StartAutoScrollTimer(nsIPresContext *aPresContext, nsIFrame *aFrame, nsPoint& aPoint, PRUint32 aDelay)
> {

This should just call GetCachedOffsetForFrame()

>Index: layout/base/src/nsCaret.cpp

>-    if (!mOptimizeDrawCaret || !mCachedOffsetValid) {
>-      mLastCaretFrame->GetPointFromOffset(presContext, mRendContext,mLastContentOffset, &mCachedFrameOffset);
>-      mCachedOffsetValid = mOptimizeDrawCaret;
>+    // if cache in nsSelection is available, apply it, else refresh it
>+    if (mOptimizeDrawCaret && mCachedOffsetValid) {
>+      nsPoint *cachedPos= &framePos;
>+      domSelection->GetOptimizeDrawCaret(nsnull, nsnull, nsnull, &cachedPos);

QI to nsISelectionPrivate, and call GetCachedOffsetForFrame().

Comment 9

[leon.zhang]

Attachment: new patch

Comment 10

[leon.zhang]

Attachment: new patch

simon: 
I am not sure there exist memory leak when "delete mCachedOffsetForFrame" in
this patch? or what is a better solution? thx

Comment 11

[Simon Fraser [no longer active]]

>Index: content/base/public/nsISelectionPrivate.idl
>===================================================================

>+
>+    /**
>+     * Returns cache for nsIFrame::GetPointFromOffset.
>+     */
>+    [noscript] void GetCachedOffsetForFrame(out PRBool aGetCachedOffset, out
nsIFrame aFrame, out PRInt32 inOffset, out nsPoint aPoint);

I don't see the need to return aGetCachedOffset to the caller. Also, aFrame
and inOffset should be [in] params. I think aPoint should be [inout]
so that you don't get a nsPoint** in the .h file.


>Index: content/base/src/nsSelection.cpp
>===================================================================

>+struct CachedOffsetForFrame {
>+  nsPoint      CachedFrameOffset;   // cached frame offset
>+  nsIFrame*    LastCaretFrame;      // store the frame the caret was last
drawn in.
>+  PRInt32      LastContentOffset;
>+};
>+

You could give this struct a constructor to initialize the fields.
Please rename the member variables using the mFoo convention.


>+  if (mCachedOffsetForFrame) {
>+    mCachedOffsetForFrame->LastCaretFrame = nsnull;
>+    delete mCachedOffsetForFrame;
>+  }

No need to null out LastCaretFrame just before deleting.

>+NS_IMETHODIMP    
>+nsTypedSelection::GetCachedOffsetForFrame(PRBool *aGetCachedOffset, nsIFrame
**aFrame, PRInt32 *inOffset, nsPoint ** aPoint)
>+{
>+  if (!mCachedOffsetForFrame) {
>+    mCachedOffsetForFrame = new CachedOffsetForFrame;
>+    mCachedOffsetForFrame->LastCaretFrame = nsnull;
>+    mCachedOffsetForFrame->LastContentOffset = nsnull;
>+    (mCachedOffsetForFrame->CachedFrameOffset).x =
(mCachedOffsetForFrame->CachedFrameOffset).y = 0;

The ctor on CachedOffsetForFrame should take care of this init.

>+    //
>+    // Retrieve the device context. We need one to create
>+    // a rendering context.
>+    //
>+
...
>+    nsCOMPtr<nsIRenderingContext> rendContext;
>+
>+    rv = deviceContext->CreateRenderingContext(closestView,
*getter_AddRefs(rendContext));
>+  
>+    if (NS_FAILED(rv))
>+      return rv;
>+
>+    if (!rendContext)
>+      return NS_ERROR_NULL_POINTER;

Since all this code is here, can we remove some code in nsCaret.cpp that
does similar stuff?

Comment 12

[leon.zhang]

Attachment: patch 1(move GetPointFromOffset to nsISelectionPrivate.idl )

Comment 13

[leon.zhang]

Attachment: patch 2

Comment 14

[leon.zhang]

1) difference between patch 1 and patch 2 is:
   move GetPointFromOffset to nsISelectionPrivate.idl, so that when drawcaret
can call this function in patch 1. This is also for future "remove some code in
nsCaret.cpp that does similar stuff?" in comments #11.

2) other part of patch 1 is almost same as patch 2:
   do most of things in comment #11, and set a flag in nsTypedSelection, so that
we  can use our cached point if necessary. 

Comment 15

[leon.zhang]

Attachment: new patch

simon: 
1) I keep orginal "nsPointPtr", because when apply "nsPointRef", will meet
complile error: 
../../../dist/include/content\nsISelectionPrivate.h(95) : error C2528:
'<Unknown>' : pointer to reference is illegal

2) other codes are based on our discussion

Comment 16

[Simon Fraser [no longer active]]

Here's how to do the idl:

Index: nsISelectionPrivate.idl
===================================================================
RCS file: /cvsroot/mozilla/content/base/public/nsISelectionPrivate.idl,v
retrieving revision 1.10
diff -u -r1.10 nsISelectionPrivate.idl
--- nsISelectionPrivate.idl	4 Apr 2003 02:36:12 -0000	1.10
+++ nsISelectionPrivate.idl	18 Apr 2003 02:34:30 -0000
@@ -45,9 +45,12 @@
 
 %{C++
 class nsIPresShell;
+class nsIFrame;
 %}
 
 [ptr] native nsIPresShell(nsIPresShell);
+[ptr] native nsIFrame(nsIFrame);
+[ref] native nsPointRef(nsPoint);
 
 [scriptable, uuid(2d5535e2-1dd2-11b2-8e38-d53ec833adf6)]
 interface nsISelectionPrivate : nsISupports
@@ -92,5 +95,9 @@
     
     /* Internal utility method to set the pres shell on a newly created
selection */
     [noscript] void setPresShell(in nsIPresShell aPresShell);
+
+
+    [noscript] void GetCachedFrameOffset(in nsIFrame aFrame, in PRInt32
aOffset, /* inout */ in nsPointRef aPoint);
+
 };
 

Comment 17

[Simon Fraser [no longer active]]

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

>Index: content/base/public/nsISelectionPrivate.idl


>+    [noscript] attribute boolean CachedFrameOffsetValid;

Leading lowercase is better here: 'cachedFrameOffsetValid'

>+    /* GetCachedOffsetForFrame
>+     * Returns cached value for nsTextFrame::GetPointFromOffset.
>+     */
>+    [noscript] void GetCachedFrameOffset(in nsIFrame aFrame, in PRInt32 inOffset, inout nsPoint aPoint);

I showed you how to get an nsPoint&  :)

>Index: content/base/src/nsSelection.cpp

>+struct CachedOffsetForFrame {
>+  CachedOffsetForFrame() {
>+    mLastCaretFrame = nsnull;
>+    mCachedFrameOffset.x = mCachedFrameOffset.y = 0;
>+    mLastContentOffset = 0;
>+  };

Nice

>+
>+  PRPackedBool mCachedFrameOffsetValid; // cached frame offset is valid?
>+  nsPoint      mCachedFrameOffset;      // cached frame offset
>+  nsIFrame*    mLastCaretFrame;         // store the frame the caret was last drawn in.
>+  PRInt32      mLastContentOffset;      // store last content offset
>+};

You'd get better packing if you put the PRPackedBool at the end, since the
nsPoint might end up 8-byte aligned. Also, the ctor needs to init
mCachedFrameOffsetValid.

>+
>+  if (mCachedOffsetForFrame) {
>+    delete mCachedOffsetForFrame;
>+  }

It's always safe to call 'delete' on a null pointer, so remove the if ().

>+NS_IMETHODIMP
>+nsTypedSelection::GetCachedFrameOffsetValid(PRBool *aCachedFrameOffsetValid)
>+{
>+  if (!aCachedFrameOffsetValid)
>+    return NS_ERROR_NULL_POINTER;

Maybe use NS_ENSURE_ARG_POINTER(aCachedFrameOffsetValid);

>+  if (mCachedOffsetForFrame)
>+    *aCachedFrameOffsetValid = mCachedOffsetForFrame->mCachedFrameOffsetValid;
>+  else
>+    *aCachedFrameOffsetValid = PR_FALSE;
>+
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP    
>+nsTypedSelection::SetCachedFrameOffsetValid(PRBool aCachedFrameOffsetValid)
>+{
>+  if (!mCachedOffsetForFrame) {
>+    mCachedOffsetForFrame = new CachedOffsetForFrame;
>+  }
>+
>+  mCachedOffsetForFrame->mCachedFrameOffsetValid = aCachedFrameOffsetValid;
>+
>+  return NS_OK;
>+}

These look good.

>+NS_IMETHODIMP    
>+nsTypedSelection::GetCachedFrameOffset(nsIFrame *aFrame, PRInt32 inOffset, nsPoint ** aPoint)
>+{
>+  if (!mCachedOffsetForFrame) {
>+    mCachedOffsetForFrame = new CachedOffsetForFrame;
>+  }
>+
>+  if (mCachedOffsetForFrame->mCachedFrameOffsetValid && 
>+      (aFrame == mCachedOffsetForFrame->mLastCaretFrame) &&
>+      (inOffset == mCachedOffsetForFrame->mLastContentOffset))
>+  {
>+     // get cached frame offset
>+     if (aPoint && *aPoint) {
>+       (*aPoint)->x = (mCachedOffsetForFrame->mCachedFrameOffset).x;
>+       (*aPoint)->y = (mCachedOffsetForFrame->mCachedFrameOffset).y;
>+     }

This will change, but you should be able to assign the point in one go:
	*aPoint = mCachedOffsetForFrame->mCachedFrameOffset;

>+     else     
>+       return NS_ERROR_NULL_POINTER;
>+  } 
>+  else
>+  {
>+     // recalculate frame offset and cache it
>+     if (aPoint && *aPoint) {
>+       GetPointFromOffset(aFrame, inOffset, *aPoint);
>+       if (mCachedOffsetForFrame->mCachedFrameOffsetValid) {

Shouldn't the cached offset always be valid just after calling
GetPointFromOffset()? If necessary, someone might have to invalidate
it by calling SetCachedFrameOffsetValid.


>Index: layout/base/src/nsCaret.cpp
>===================================================================

>   if (!mDrawn)
>   {
>+    nsPoint   framePos(0, 0);
>+    nsPoint   *cachedPos= &framePos;
>     nsRect    caretRect = frameRect;
>+    PRBool    aCachedFrameOffsetValid = PR_FALSE;
>+    nsCOMPtr<nsISelection> domSelection = do_QueryReferent(mDomSelectionWeak);
>+    nsCOMPtr<nsISelectionPrivate> privateSelection = do_QueryInterface(domSelection);
> 
>-    // if use cache and cache is ready, apply it, else refresh it
>-    if (!mOptimizeDrawCaret || !mCachedOffsetValid) {
>-      mLastCaretFrame->GetPointFromOffset(presContext, mRendContext,mLastContentOffset, &mCachedFrameOffset);
>-      mCachedOffsetValid = mOptimizeDrawCaret;
>-    }
>+    // if cache in selection is available, apply it, else refresh it
>+    privateSelection->GetCachedFrameOffsetValid(&aCachedFrameOffsetValid);
>+    if (aCachedFrameOffsetValid)
>+      privateSelection->GetCachedFrameOffset(mLastCaretFrame,mLastContentOffset, &cachedPos);

Can't we just assume that that privateSelection->GetCachedFrameOffset() will
always return a valid offset, because it recomputed it if necessary? This
requires that SetCachedFrameOffsetValid(PR_FALSE) has been called
appropriately.

This goes back to the old issue of knowing when a frame got reflowed.

If we have to stay with this model of "assume it's invalid unless told
otherwise", then I think SetCachedFrameOffsetValid() needs to be renamed to
CanCacheFrameOffset().

> nsresult
>Index: editor/libeditor/base/nsEditor.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/base/nsEditor.cpp,v
>retrieving revision 1.390
>diff -u -r1.390 nsEditor.cpp
>--- editor/libeditor/base/nsEditor.cpp	2 Apr 2003 05:48:08 -0000	1.390
>+++ editor/libeditor/base/nsEditor.cpp	17 Apr 2003 11:08:55 -0000
>@@ -746,18 +746,16 @@
>   NS_PRECONDITION(mPlaceHolderBatch > 0, "zero or negative placeholder batch count when ending batch!");
>   if (mPlaceHolderBatch == 1)
>   {
>-    nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak);
>-    nsCOMPtr<nsICaret> caret;
>-    if (!ps)
>-      return NS_ERROR_FAILURE;
>-
>-    nsresult rv = ps->GetCaret(getter_AddRefs(caret));
>+    nsCOMPtr<nsISelection>selection;
>+    nsresult rv = GetSelection(getter_AddRefs(selection));
>     if (NS_FAILED(rv))
>       return rv;
> 
>-    // optimizing drawcaret starts
>-    if (caret) {
>-      caret->SetOptimizeDrawCaret(PR_TRUE);
>+    nsCOMPtr<nsISelectionPrivate>selPrivate(do_QueryInterface(selection));
>+
>+    // Set cache for frame offset valid
>+    if (selPrivate) {
>+      selPrivate->SetCachedFrameOffsetValid(PR_TRUE);

You need some comments here to state what your assumptions are about the order
in which things are called, so that you know whether the cached offset is
valid.

Comment 18

[leon.zhang]

Attachment: new version

Comment 19

[Simon Fraser [no longer active]]

Comment on attachment 120940 [details] [diff] [review]
new version

>Index: content/base/public/nsISelectionPrivate.idl
>===================================================================
>+    /* GetCachedOffsetForFrame
>+     * Returns cached value for nsTextFrame::GetPointFromOffset.
>+     */
>+    [noscript] void GetCachedFrameOffset(in nsIFrame aFrame, in PRInt32 inOffset, in nsPointRef aPoint);

This could use leading lowercase, like other idl methods. It gets uppercased in
 the .h file.

>+struct CachedOffsetForFrame {
>+  CachedOffsetForFrame() {
>+    mCachedFrameOffset.x = mCachedFrameOffset.y = 0;
>+    mLastCaretFrame = nsnull;
>+    mLastContentOffset = 0;
>+    mCanCacheFrameOffset = PR_FALSE;
>+  };

This would be cleaner using initializers:

  CachedOffsetForFrame()
  : mCachedFrameOffset(0, 0) // nsPoint ctor
  , mLastCaretFrame(nsnull)
  , mLastContentOffset(0)
  , mCanCacheFrameOffset(PR_FALSE)
  {}


>+NS_IMETHODIMP
>+nsTypedSelection::GetCanCacheFrameOffset(PRBool *aCanCacheFrameOffset)
>+{ 
>+  if (mCachedOffsetForFrame && mCachedOffsetForFrame)

This line seems wrong.

>Index: layout/base/src/nsCaret.cpp
>===================================================================

>+    // if cache in selection is available, apply it, else refresh it
>+    privateSelection->GetCanCacheFrameOffset(&aCanCacheFrameOffset);
>+    if (aCanCacheFrameOffset)

You don't need to do that test here, since
nsTypedSelection::GetCachedFrameOffset() does it for you. So you can always
just go ahead and call GetCachedFrameOffset().

>Index: editor/libeditor/base/nsEditor.cpp
>===================================================================

>-    // optimizing drawcaret starts
>-    if (caret) {
>-      caret->SetOptimizeDrawCaret(PR_TRUE);
>+    nsCOMPtr<nsISelectionPrivate>selPrivate(do_QueryInterface(selection));
>+
>+    // Cache begin to be available only if reflow/view-refresh/scroll happen sequentially during current 
>+    // process resulted from changing content of DOM through UI (e.g. typing chars into input fields).
>+    // This soultion can prevent exceptions which cause similar sequence during other process(e.g. changing
>+    // content of DOM by means of javascript). See bugs 199412 35296
>+    if (selPrivate) {
>+      selPrivate->SetCanCacheFrameOffset(PR_TRUE);
>     }

I think this comments needs a bit of work. How about:

// By making the assumption that no reflow happens during the calls
// to EndUpdateViewBatch and ScrollSelectionIntoView, we are able to
// allow the selection to cache a frame offset which is used by the
// caret drawing code. We only enable this cache here; at other times,
// we have no way to know whether reflow invalidates it
// See bugs 35296 and 199412.

Fix those, and r/sr=sfraser

Comment 20

[leon.zhang]

Attachment: final patch(smfr's comments + newest trunk)

transfer r/sr to this one

Comment 21

[leon.zhang]

The patch can decrease time consumed by nsTextFrame::GetPointFromOffset about
half: from 4.2% to 2.2%.

Comment 22

[leon.zhang]

more testing data:
                 orignal codes      current patch    
nsPresShell::      5.71% / 5.59%     5.92% /5.92% 
SetCaretEnabled        *5.65%*           *5.92%*    

nsCaret::          10.42% / 10.44%   10.77% / 11.03%
SetCaretVisible        *11.43%*           *10.80%*    

nsCaret::          5.76% / 5.74%     6.23% / 6.26% 
DrawCaret             *5.75%*            *6.25%*    

nsTextFrame::      4.22% / 4.21%     2.22% / 2.24% 
GetPointFromOffset    *4.22%*            *2.23%*

Comment 23

[leon.zhang]

testing data of other functions in table above change a little is related to
current data structure(cache moved from caret to selection)

Comment 24

[leon.zhang]

Other functions(SetCaretEnabled,SetCaretVisible,DrawCaret) in table consumed a
little more cpu time than previous codes.  This result is related to current
code structure(cache has been moved from caret to selection so that both
selection and caret can use the same cache based on a better code stlyle).

Comment 25

[leon.zhang]

Comment on attachment 121053 [details] [diff] [review]
final patch(smfr's comments + newest trunk)

checked in

Comment 26

[leon.zhang]

fixed