Closed Bug 199412 Opened 21 years ago Closed 21 years ago

optimize nsTypedSelection::ScrollIntoView()

Categories

(Core :: DOM: Selection, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: leon.zhang, Assigned: leon.zhang)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 8 obsolete files)

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.
assigned to me
Assignee: mjudge → leon.zhang
After patche for bug 35206 has been committed, I will give a patch based on this
Depends on: 35296
Blocks: 188318
typo:
After patch for bug 35296 has been committed, I will give a patch based on that bug.
Attached patch patch 1.0 (obsolete) — Splinter Review
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.
Attachment #119172 - Attachment is obsolete: true
Attached patch test patch (obsolete) — Splinter Review
Keywords: perf
Attachment #119811 - Attachment is obsolete: true
Attachment #119955 - Flags: superreview?
Attachment #119955 - Flags: review?(sfraser)
Attachment #119955 - Flags: superreview? → superreview?(sfraser)
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().
Attachment #119955 - Flags: superreview?(sfraser)
Attachment #119955 - Flags: superreview-
Attachment #119955 - Flags: review?(sfraser)
Attachment #119955 - Flags: review-
Attached patch new patch (obsolete) — Splinter Review
Attachment #119955 - Attachment is obsolete: true
Attachment #120067 - Attachment is obsolete: true
Attached patch new patch (obsolete) — Splinter Review
simon: 
I am not sure there exist memory leak when "delete mCachedOffsetForFrame" in
this patch? or what is a better solution? thx
>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?

Attachment #120068 - Attachment is obsolete: true
Attached patch patch 2 (obsolete) — Splinter Review
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. 
Attachment #120191 - Attachment is obsolete: true
Attached patch new patch (obsolete) — Splinter Review
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
Attachment #120193 - Attachment is obsolete: true
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 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.
Attached patch new versionSplinter Review
Attachment #120813 - Attachment is obsolete: true
Attachment #120940 - Flags: superreview?(sfraser)
Attachment #120940 - Flags: review?(sfraser)
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
Attachment #120940 - Flags: superreview?(sfraser)
Attachment #120940 - Flags: superreview+
Attachment #120940 - Flags: review?(sfraser)
Attachment #120940 - Flags: review+
transfer r/sr to this one
Attachment #121053 - Attachment description: final patch → final patch(smfr's comments + newest trunk)
The patch can decrease time consumed by nsTextFrame::GetPointFromOffset about
half: from 4.2% to 2.2%.
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%*
testing data of other functions in table above change a little is related to
current data structure(cache moved from caret to selection)
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 on attachment 121053 [details] [diff] [review]
final patch(smfr's comments + newest trunk)

checked in
fixed
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.