204753 - [addrbook] optimize dir_GetChildList

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[[e-mail]]

pasted from bug 203859:

this is a small optimization to the function dir_GetChildList we created. as it
stands, for a "typical end-user" (in this case, me ;) addrbook setup, the
number of entries returned by nsIPrefBranch::GetChildList is 60-odd, but the
number of unique entries is only 15. so memmoving for every non-unique entry is
quite slow - better approach is to move only unique entries (which is a simple
ptr assignment). yeah, i know this isn't really important in the scheme of
things, but i felt guilty about writing slow code. ;)

i've tested this and it looks to work just fine. it's not a biggie so it can
wait 'til whenever is convenient to land.

Comment 1

[[e-mail]]

Attachment: the fix

Comment 2

[[e-mail]]

Comment on attachment 122678 [details] [diff] [review]
the fix

hitting up caillon for review.

Comment 3

[[e-mail]]

Attachment: diff -y

much more readable diff -y

to keep reviewers friendly.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 122678 [details] [diff] [review]
the fix

>+        PRUint32 cur = 0;
>+        PRUint32 next = 1;
>         do {

How about converting this to a for() loop?

>-            // if the elements are equal, move the trailing portion of the array
>-            // forward a notch.
>-            if (!comparePrefArrayMembers(&((*aChildList)[cur]), &((*aChildList)[prev]), &branchLen)) {
>+            // if the elements are equal, just increment the next element ptr
>+            // and continue. else, move the element ptr.


How about splitting this comment off?  Put the first part inside the if() {}
and the second part inside the else{}.	e.g.

  if (foo) {
    // The elements are ...
  } else {
    // Just move the ptr
  }


>+            if (!comparePrefArrayMembers(&((*aChildList)[cur]), &((*aChildList)[next]), &branchLen)) {
>                 // free up memory.
>-                nsMemory::Free((*aChildList)[cur]);
>-                // move previous elements forward a notch
>-                memmove(&((*aChildList)[cur]), &((*aChildList)[prev]), (*aCount - prev) * sizeof(char*));
>-                // decrement the count
>-                --(*aCount);
>+                nsMemory::Free((*aChildList)[next]);
>+            } else {
>+                // cur & next are unique - do the assignment and increment both.
>+                ++cur; // this points to the next free location now
>+                (*aChildList)[cur] = (*aChildList)[next]; // it's okay if cur == next here

(*aChildList)[++cur]  maybe?  With some comment mangling perhaps.


> 
>     return NS_OK;

Comment 5

[[e-mail]]

Attachment: v2

Comment 6

[[e-mail]]

Attachment: v2 diff -yw

Comment 7

[[e-mail]]

Attachment: v2.1

uh no... the previous one was wrong. sorry for spam :/

Comment 8

[[e-mail]]

Attachment: v2.1 diff -y

Comment 9

[[e-mail]]

Comment on attachment 124809 [details] [diff] [review]
v2.1

carrying over r=caillon, requesting sr.

addresses caillon's comments.

Comment 10

[[e-mail]]

Comment on attachment 124809 [details] [diff] [review]
v2.1

kicking this over to bienvenu since dmose isn't around. feel free to kick it
back ;)

Comment 11

[David :Bienvenu]

Comment on attachment 124809 [details] [diff] [review]
v2.1

sr=bienvenu

Comment 12

[[e-mail]]

thx!

fixed on trunk

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified, lxr-style.