293084 - [MailNews: Search] minor perf improvement cache GetSize in nsMsgSearchNews::DuplicateHit

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[timeless]

	nsUInt32Array::GetSize returned	0x00004199	unsigned int

calling GetSize 4000 times seems excessive

>	msgbase.dll!nsMsgSearchNews::DuplicateHit(unsigned int artNum=0x00004635) 
Line 357 + 0x1e	C++
 	msgbase.dll!nsMsgSearchNews::CollateHits()  Line 385 + 0xc	C++
 	msgbase.dll!nsMsgSearchNews::CurrentUrlDone(int exitCode=0x00000000)  Line 327	C++
 	msgbase.dll!nsMsgSearchSession::OnStopRunningUrl(nsIURI * url=0x06f2de9c,
unsigned int aExitCode=0x00000000)  Line 375	C++
 	msgbase.dll!nsUrlListenerManager::BroadcastChange(nsIURI * aUrl=0x06f2de9c,
nsUrlNotifyType notification=nsUrlNotifyStopRunning, unsigned int
aErrorCode=0x00000000)  Line 110	C++
 	msgbase.dll!nsUrlListenerManager::OnStopRunningUrl(nsIMsgMailNewsUrl *
aUrl=0x06f2de9c, unsigned int aErrorCode=0x00000000)  Line 123 + 0x12	C++
 	msgbsutl.dll!nsMsgMailNewsUrl::SetUrlState(int aRunningUrl=0x00000000,
unsigned int aExitCode=0x00000000)  Line 132	C++
 	msgnews.dll!nsNNTPProtocol::CleanupAfterRunningUrl()  Line 5218	C++
 	msgnews.dll!nsNNTPProtocol::ProcessProtocolState(nsIURI * url=0x06f2de9c,
nsIInputStream * inputStream=0x06f32ae0, unsigned int sourceOffset=0x0018f962,
unsigned int length=0x000009eb)  Line 5133 + 0xb	C++
 	msgbsutl.dll!nsMsgProtocol::OnDataAvailable(nsIRequest * request=0x06f32f30,
nsISupports * ctxt=0x06f2de98, nsIInputStream * inStr=0x06f32ae0, unsigned int
sourceOffset=0x0018f962, unsigned int count=0x000009eb)  Line 350 + 0x20	C++
 	necko.dll!nsInputStreamPump::OnStateTransfer()  Line 437 + 0x46	C++

Comment 1

[timeless]

slightly more dangerous:
also calls GetSize 10000 times

>	msgbase.dll!nsMsgSearchNews::ReportHits()  Line 435	C++
 	msgbase.dll!nsMsgSearchNews::CurrentUrlDone(int exitCode=0x00000000)  Line 328	C++
 	msgbase.dll!nsMsgSearchSession::OnStopRunningUrl(nsIURI * url=0x06f2de9c,
unsigned int aExitCode=0x00000000)  Line 375	C++
 	msgbase.dll!nsUrlListenerManager::BroadcastChange(nsIURI * aUrl=0x06f2de9c,
nsUrlNotifyType notification=nsUrlNotifyStopRunning, unsigned int
aErrorCode=0x00000000)  Line 110	C++
 	msgbase.dll!nsUrlListenerManager::OnStopRunningUrl(nsIMsgMailNewsUrl *
aUrl=0x06f2de9c, unsigned int aErrorCode=0x00000000)  Line 123 + 0x12	C++
 	msgbsutl.dll!nsMsgMailNewsUrl::SetUrlState(int aRunningUrl=0x00000000,
unsigned int aExitCode=0x00000000)  Line 132	C++
 	msgnews.dll!nsNNTPProtocol::CleanupAfterRunningUrl()  Line 5218	C++
 	msgnews.dll!nsNNTPProtocol::ProcessProtocolState(nsIURI * url=0x06f2de9c,
nsIInputStream * inputStream=0x06f32ae0, unsigned int sourceOffset=0x0018f962,
unsigned int length=0x000009eb)  Line 5133 + 0xb	C++
 	msgbsutl.dll!nsMsgProtocol::OnDataAvailable(nsIRequest * request=0x06f32f30,
nsISupports * ctxt=0x06f2de98, nsIInputStream * inStr=0x06f32ae0, unsigned int
sourceOffset=0x0018f962, unsigned int count=0x000009eb)  Line 350 + 0x20	C++

Comment 2

[Eyal Rozenberg]

Umm, what is this bug about exactly?

Comment 3

[Wayne Mery (:wsmwk)]

(In reply to comment #2)
> Umm, what is this bug about exactly?

timeless?

Comment 4

[Bruno 'Aqualon' Escherl]

Attachment: Cache result from GetSize() + code cleanup

The patch caches the GetSize() calls in ReportHits() and DuplicateHit() like it's already done in CollateHits(). Besides that it also does a little bit code cleanup.

Comment 5

[Karsten Düsterloh]

Comment on attachment 284840 [details] [diff] [review]
Cache result from GetSize() + code cleanup

Basically fine, just some nits:

>Index: mailnews/base/search/src/nsMsgSearchNews.cpp
>===================================================================
>   }
> }
> #endif // 0
> PRBool nsMsgSearchNews::DuplicateHit(PRUint32 artNum)
> // ASSUMES m_hits is sorted!!
> {

Add an empty line before the function, move the comment into the body.
Probably better text then "Assert that m_hits is sorted!".

>+  PRUint32 size = m_hits.GetSize();
>   PRUint32 index;
>-  for (index = 0; index < m_hits.GetSize(); index++)
>+  for (index = 0; index < size; ++index)

Move the index definition into the for statement.

>+void nsMsgSearchNews::CollateHits()
> {
...
>   PRUint32 termCount;
>   m_searchTerms->Count(&termCount);
>   PRUint32 candidateCount = 0;
>-  while (index < size)
>+  for (PRUint32 index = 0; index < size; ++index)   
>   {
>     if (candidate == m_candidateHits.ElementAt(index))
>       candidateCount++;
>     else
>       candidateCount = 1;
>+    candidate = m_candidateHits.ElementAt(index);

Read candidateHits[index] once before comparing/using it. 
Use ++candidateCount.
Only assign candidate a new value if it changed.

Comment 6

[Bruno 'Aqualon' Escherl]

Attachment: Fix review nits

Patch fixes all nits from comment 5.

Comment 7

[David :Bienvenu]

Comment on attachment 284873 [details] [diff] [review]
Fix review nits

looks ok, thx.

Comment 8

[Karsten Düsterloh]

Landed on trunk.