293088 - Changing folders results in thousands of calls to nsTreeBoxObject::InvalidateRange

Status: RESOLVED FIXED

(MailNews Core :: Database, defect, P5)

Description

[timeless]

proposal:
nsMsgDatabase::ClearNewList could call nsMsgDBView::Observe with a message that
means "you want to call nsTreeBoxObject::BeginUpdateBatch" before it loops
around calling nsMsgDBView::NoteChange and then call nsMsgDBView::Observe with a
message that means "you want to call nsTreeBoxObject::EndUpdateBatch".

 	gklayout.dll!nsTreeBoxObject::GetTreeBody()  Line 147 + 0x3f	C++
 	gklayout.dll!nsTreeBoxObject::InvalidateRange(int aStart=0x000082be, int
aEnd=0x000082be)  Line 339 + 0x8	C++
 	msgbase.dll!nsMsgDBView::NoteChange(unsigned int firstLineChanged=0x000082be,
int numChanged=0x00000001, int changeType=0x00000002)  Line 4708	C++
>	msgbase.dll!nsMsgDBView::OnHdrChange(nsIMsgDBHdr * aHdrChanged=0x06f3b558,
unsigned int aOldFlags=0x00010010, unsigned int aNewFlags=0x00000010,
nsIDBChangeListener * aInstigator=0x00000000)  Line 4611	C++
 	msgdb.dll!nsMsgDatabase::NotifyHdrChangeAll(nsIMsgDBHdr *
aHdrChanged=0x06f3b558, unsigned int oldFlags=0x00010010, unsigned int
newFlags=0x00000010, nsIDBChangeListener * instigator=0x00000000)  Line 603 +
0x27	C++
 	msgdb.dll!nsMsgDatabase::ClearNewList(int notify=0x00000001)  Line 2494	C++
 	msgbsutl.dll!nsMsgDBFolder::ClearNewMessages()  Line 466 + 0x1f	C++
 	xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x0541d720, unsigned int
methodIndex=0x00000039, unsigned int paramCount=0x00000000, nsXPTCVariant *
params=0x0012c0e4)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2068 + 0x1e	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x03df9ea8, JSObject *
obj=0x04a0a018, unsigned int argc=0x00000000, long * argv=0x06ff88b4, long *
vp=0x0012c3b8)  Line 1311 + 0xb	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x03df9ea8, unsigned int argc=0x00000000,
unsigned int flags=0x00000000)  Line 1320 + 0x20	C
 	js3250.dll!js_Interpret(JSContext * cx=0x03df9ea8, unsigned char *
pc=0x03b19ad9, long * result=0x0012cea4)  Line 3614 + 0xf	C
 	js3250.dll!js_Invoke(JSContext * cx=0x03df9ea8, unsigned int argc=0x00000001,
unsigned int flags=0x00000002)  Line 1340 + 0x13	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x03df9ea8, JSObject *
obj=0x04a08da8, long fval=0x03656b48, unsigned int flags=0x00000000, unsigned
int argc=0x00000001, long * argv=0x0012d19c, long * rval=0x0012d1a0)  Line
1417 + 0x14	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x03df9ea8, JSObject *
obj=0x04a08da8, long fval=0x03656b48, unsigned int argc=0x00000001, long *
argv=0x0012d19c, long * rval=0x0012d1a0)  Line 3878 + 0x1f	C
 	gklayout.dll!nsJSContext::CallEventHandler(JSObject * aTarget=0x04a08da8,
JSObject * aHandler=0x03656b48, unsigned int argc=0x00000001, long *
argv=0x0012d19c, long * rval=0x0012d1a0)  Line 1401 + 0x21	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x05d9f9e8) 
Line 205 + 0x2d	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct *
aListenerStruct=0x04ae15d0, nsIDOMEvent * aDOMEvent=0x05d9f9e8,
nsIDOMEventTarget * aCurrentTarget=0x061de458, unsigned int aSubType=0x00000008,
unsigned int aPhaseFlags=0x00000007)  Line 1557 + 0x14	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext *
aPresContext=0x046ab0a0, nsEvent * aEvent=0x0012d698, nsIDOMEvent * *
aDOMEvent=0x0012d644, nsIDOMEventTarget * aCurrentTarget=0x061de458, unsigned
int aFlags=0x00000007, nsEventStatus * aEventStatus=0x0012d694)  Line 1656	C++
 	gklayout.dll!nsXULElement::HandleDOMEvent(nsPresContext *
aPresContext=0x046ab0a0, nsEvent * aEvent=0x0012d698, nsIDOMEvent * *
aDOMEvent=0x0012d644, unsigned int aFlags=0x00000007, nsEventStatus *
aEventStatus=0x0012d694)  Line 2192	C++
 	gklayout.dll!nsTreeSelection::FireOnSelectHandler()  Line 760	C++
 	gklayout.dll!nsTreeSelection::Select(int aIndex=0x00000005)  Line 377	C++
 	xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x04a59d38, unsigned int
methodIndex=0x00000008, unsigned int paramCount=0x00000001, nsXPTCVariant *
params=0x0012d840)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2068 + 0x1e	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x03df9ea8, JSObject *
obj=0x03803eb0, unsigned int argc=0x00000001, long * argv=0x06ff862c, long *
vp=0x0012db14)  Line 1311 + 0xb	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x03df9ea8, unsigned int argc=0x00000001,
unsigned int flags=0x00000000)  Line 1320 + 0x20	C
 	js3250.dll!js_Interpret(JSContext * cx=0x03df9ea8, unsigned char *
pc=0x076d4371, long * result=0x0012e600)  Line 3614 + 0xf	C
 	js3250.dll!js_Invoke(JSContext * cx=0x03df9ea8, unsigned int argc=0x00000001,
unsigned int flags=0x00000002)  Line 1340 + 0x13	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x03df9ea8, JSObject *
obj=0x04a09098, long fval=0x03345c20, unsigned int flags=0x00000000, unsigned
int argc=0x00000001, long * argv=0x0012e8f8, long * rval=0x0012e8fc)  Line
1417 + 0x14	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x03df9ea8, JSObject *
obj=0x04a09098, long fval=0x03345c20, unsigned int argc=0x00000001, long *
argv=0x0012e8f8, long * rval=0x0012e8fc)  Line 3878 + 0x1f	C
 	gklayout.dll!nsJSContext::CallEventHandler(JSObject * aTarget=0x04a09098,
JSObject * aHandler=0x03345c20, unsigned int argc=0x00000001, long *
argv=0x0012e8f8, long * rval=0x0012e8fc)  Line 1401 + 0x21	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x06618df8) 
Line 205 + 0x2d	C++
 	gklayout.dll!nsXBLPrototypeHandler::ExecuteHandler(nsIDOMEventReceiver *
aReceiver=0x061df040, nsIDOMEvent * aEvent=0x06618df8)  Line 492	C++
 	gklayout.dll!nsXBLEventHandler::HandleEvent(nsIDOMEvent * aEvent=0x06618df8) 
Line 85	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct *
aListenerStruct=0x04859ac8, nsIDOMEvent * aDOMEvent=0x06618df8,
nsIDOMEventTarget * aCurrentTarget=0x061df040, unsigned int aSubType=0x00000001,
unsigned int aPhaseFlags=0x00000007)  Line 1557 + 0x14	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext *
aPresContext=0x046ab0a0, nsEvent * aEvent=0x0012f7a8, nsIDOMEvent * *
aDOMEvent=0x0012f368, nsIDOMEventTarget * aCurrentTarget=0x061df040, unsigned
int aFlags=0x00000007, nsEventStatus * aEventStatus=0x0012f53c)  Line 1656	C++
 	gklayout.dll!nsXULElement::HandleDOMEvent(nsPresContext *
aPresContext=0x046ab0a0, nsEvent * aEvent=0x0012f7a8, nsIDOMEvent * *
aDOMEvent=0x0012f368, unsigned int aFlags=0x00000007, nsEventStatus *
aEventStatus=0x0012f53c)  Line 2192	C++
 	gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f7a8,
nsIView * aView=0x04a9d9c0, unsigned int aFlags=0x00000001, nsEventStatus *
aStatus=0x0012f53c)  Line 6298 + 0x31	C++
 	gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x04a9d9c0, nsGUIEvent *
aEvent=0x0012f7a8, nsEventStatus * aEventStatus=0x0012f53c, int
aForceHandle=0x00000000, int & aHandled=0x00000001)  Line 6142 + 0x19	C++
 	gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x04a9d9c0, nsGUIEvent
* aEvent=0x0012f7a8, int aCaptured=0x00000000)  Line 2506	C++
 	gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012f7a8,
nsEventStatus * aStatus=0x0012f684)  Line 2228 + 0x14	C++
 	gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f7a8)  Line 174	C++
 	gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f7a8,
nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1181 + 0xa	C++
 	gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012f7a8) 
Line 1202	C++
 	gkwidget.dll!nsWindow::DispatchMouseEvent(unsigned int aEventType=0x0000012e,
unsigned int wParam=0x00000001, nsPoint * aPoint=0x00000000)  Line 5903 + 0x15	C++
 	gkwidget.dll!ChildWindow::DispatchMouseEvent(unsigned int
aEventType=0x0000012e, unsigned int wParam=0x00000001, nsPoint *
aPoint=0x00000000)  Line 6159	C++
 	gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=0x00000201, unsigned
int wParam=0x00000001, long lParam=0x0067002a, long * aRetValue=0x0012fc7c) 
Line 4547 + 0x1c	C++
 	gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x011a2068, unsigned int
msg=0x00000201, unsigned int wParam=0x00000001, long lParam=0x0067002a)  Line
1473 + 0x1b	C++
 	user32.dll!_InternalCallWinProc@20()  + 0x28	
 	user32.dll!_UserCallWinProcCheckWow@32()  + 0xb7	
 	user32.dll!_DispatchMessageWorker@8()  + 0xdc	
 	user32.dll!_DispatchMessageW@4()  + 0xf	
 	gkwidget.dll!nsAppShell::Run()  Line 135	C++

Comment 1

[timeless]

there's sorta an api for this:
nsMsgDBView::EnableChangeUpdates
nsMsgDBView::DisableChangeUpdates

and a really strange function pair:
nsMsgDBView::NoteStartChange - nop
nsMsgDBView::NoteEndChange

this also applies to loading:
 	gklayout.dll!mozAutoDocUpdate::mozAutoDocUpdate(nsIDocument *
aDocument=0x03b80c38, unsigned int aUpdateType=1, int aNotify=1)  Line 738 +
0x22	C++
 	gklayout.dll!nsXULElement::SetAttrAndNotify(int aNamespaceID=0, nsIAtom *
aAttribute=0x01818070, nsIAtom * aPrefix=0x00000000, const nsAString &
aOldValue={...}, nsAttrValue & aParsedValue={...}, int aModification=1, int
aFireMutation=0, int aNotify=1)  Line 1515	C++
 	gklayout.dll!nsXULElement::SetAttr(int aNamespaceID=0, nsIAtom *
aName=0x01818070, nsIAtom * aPrefix=0x00000000, const nsAString & aValue={...},
int aNotify=1)  Line 1495 + 0x34	C++
 	gklayout.dll!nsIContent::SetAttr(int aNameSpaceID=0, nsIAtom *
aName=0x01818070, const nsAString & aValue={...}, int aNotify=1)  Line 285	C++
 	gklayout.dll!nsTreeBodyFrame::InvalidateScrollbar()  Line 805	C++
 	gklayout.dll!nsTreeBodyFrame::RowCountChanged(int aIndex=25449, int aCount=1)
 Line 1422	C++
 	gklayout.dll!nsTreeBoxObject::RowCountChanged(int aIndex=25449, int aDelta=1)
 Line 385 + 0x14	C++
 	msgbase.dll!nsMsgDBView::NoteChange(unsigned int firstLineChanged=25449, int
numChanged=1, int changeType=1)  Line 4739	C++
 	msgbase.dll!nsMsgDBView::AddHdr(nsIMsgDBHdr * msgHdr=0x0627f2a8)  Line 4378	C++
>	msgbase.dll!nsMsgThreadedDBView::AddMsgToThreadNotInView(nsIMsgThread *
threadHdr=0x0600cb88, nsIMsgDBHdr * msgHdr=0x0627f2a8, int ensureListed=0)  Line
747 + 0x12	C++
 	msgbase.dll!nsMsgThreadedDBView::OnNewHeader(nsIMsgDBHdr * newHdr=0x0627f2a8,
unsigned int aParentKey=4294967295, int ensureListed=0)  Line 684	C++
 	msgbase.dll!nsMsgDBView::OnHdrAdded(nsIMsgDBHdr * aHdrChanged=0x0627f2a8,
unsigned int aParentKey=4294967295, int aFlags=65536, nsIDBChangeListener *
aInstigator=0x00000000)  Line 4665	C++
 	msgdb.dll!nsMsgDatabase::NotifyHdrAddedAll(nsIMsgDBHdr * aHdrAdded=0x0627f2a8,
unsigned int parentKey=4294967295, int flags=65536, nsIDBChangeListener *
instigator=0x00000000)  Line 684 + 0x27	C++
 	msgdb.dll!nsMsgDatabase::AddNewHdrToDB(nsIMsgDBHdr * newHdr=0x0627f2a8, int
notify=1)  Line 2998	C++
 	msgnews.dll!nsNNTPNewsgroupList::ParseLine(char * line=0x00000000, unsigned
int * message_number=0x0012fb08)  Line 708 + 0x31	C++

Comment 2

[timeless]

the same applies to nsMsgDatabase::MarkAllRead and similar ilk:

 	msgbase.dll!nsMsgDBView::NoteChange(unsigned int firstLineChanged=288, int
numChanged=1, int changeType=2)  Line 4728	C++
 	msgbase.dll!nsMsgDBView::OnHdrChange(nsIMsgDBHdr * aHdrChanged=0x06786dc0,
unsigned int aOldFlags=65552, unsigned int aNewFlags=17, nsIDBChangeListener *
aInstigator=0x00000000)  Line 4646	C++
 	msgdb.dll!nsMsgDatabase::NotifyHdrChangeAll(nsIMsgDBHdr *
aHdrChanged=0x06786dc0, unsigned int oldFlags=65552, unsigned int newFlags=17,
nsIDBChangeListener * instigator=0x00000000)  Line 606 + 0x27	C++
 	msgdb.dll!nsMsgDatabase::MarkHdrReadInDB(nsIMsgDBHdr * msgHdr=0x06786dc0, int
bRead=1, nsIDBChangeListener * instigator=0x00000000)  Line 2039	C++
 	msgdb.dll!nsMsgDatabase::MarkHdrRead(nsIMsgDBHdr * msgHdr=0x06786dc0, int
bRead=1, nsIDBChangeListener * instigator=0x00000000)  Line 2357 + 0x1a	C++
>	msgdb.dll!nsMsgDatabase::MarkAllRead(nsMsgKeyArray * thoseMarked=0x00000000) 
Line 2405 + 0x14	C++

Comment 3

[Wayne Mery (:wsmwk)]

Is this worth doing/taking for TB 3?

Comment 4

[Ludovic Hirlimann [:Usul]]

(In reply to comment #3)
> Is this worth doing/taking for TB 3?

I think that there is little chance for that to be taken into account for TB3.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

My suspicion is that this is fairly unlikely to block, though it's not really possible to say without some understanding of which/how many users are affected by this and how much.  bienvenu may have that understanding in his head, however.

Comment 6

[Wayne Mery (:wsmwk)]

how does number of calls scale?  Is it proportional to number of messages, or number of threads?  And is it only for the messages in the view?  Or for all messages in the folder?

Comment 7

[David :Bienvenu]

I think adding batching for this is something worth investigating.

Comment 8

[David :Bienvenu]

The original issue reported only affects users who enter a folder with a huge number of "new" messages and leave without reading those new messages. So it's not a normal occurrence. Similarly, if you have mark all read on exit set and leave a folder with a lot of unread messages, that will also generate a lot of invalidates. I think the mark all read on exit pref is a hidden pref, fwiw. But in any case, I agree that we should expose and use the batching methods.

Comment 9

[David :Bienvenu]

Attachment: enable disabling of updates via idl - checked in

this allows us to disable updates from js (which TB needs, not sure about SM) and disables updates internally for mark all read.

Comment 10

[[e-mail]]

Comment on attachment 525072 [details] [diff] [review]
enable disabling of updates via idl - checked in

>+  /// Suppress change notifications
>+  attribute boolean suppressChangeNotifications;
What's the difference between this and using Begin/EndUpdateBatch on the tree?

Comment 11

[David :Bienvenu]

(In reply to comment #10)
> Comment on attachment 525072 [details] [diff] [review]
> enable disabling of updates via idl
> 
> >+  /// Suppress change notifications
> >+  attribute boolean suppressChangeNotifications;
> What's the difference between this and using Begin/EndUpdateBatch on the tree?

We'll avoid all the calls to invalidate range, so basically it saves unneeded function calls on the tree.

Comment 12

[[e-mail]]

Comment on attachment 525072 [details] [diff] [review]
enable disabling of updates via idl - checked in

>+  /// Suppress change notifications
>+  attribute boolean suppressChangeNotifications;
Maybe a comment that this is faster but less safe than Begin/EndUpdateBatch
(i.e. you have to manage your own invalidation and row count changes) ?

Comment 13

[David :Bienvenu]

(In reply to comment #12)

> >+  /// Suppress change notifications
> >+  attribute boolean suppressChangeNotifications;
> Maybe a comment that this is faster but less safe than Begin/EndUpdateBatch
> (i.e. you have to manage your own invalidation and row count changes) ?

yeah, that's an excellent idea - I'll do that.

Comment 14

[David :Bienvenu]

Attachment: proposed fix

since we're leaving the folder, suppressing changing notifications seems reasonable. I don't know of any easy way to check this with mozmill since we're essentially just removing calls to invalidateRange on the tree.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

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

Yeah, no need to test this as it is a performance optimization and our love of throwing away dbViews is well known so a functional regression is not likely.  (And the performance optimization's regression would ideally be caught via a talos-like solution, not a one-off proxy that counts invocations.)

However, could you add a comment like the following, please:
// Suppress useless InvalidateRange calls to the tree by the dbView.

Comment 16

[David :Bienvenu]

comment added, thx, - http://hg.mozilla.org/comm-central/rev/cc71bd856f9f

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

I pushed a bustage fix since the mozmill tests all got super mad and the likely problem seemed obvious (not checking whether this.dbView was non-null):
http://hg.mozilla.org/comm-central/rev/5dec9f6d5b21

Comment 18

[David :Bienvenu]

ugh, thx - hence the try catch, perhaps.