[addrbook] optimize dir_GetChildList

VERIFIED FIXED

Status

SeaMonkey
MailNews: Address Book & Contacts
--
minor
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: dwitte@gmail.com, Assigned: dwitte@gmail.com)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
Created attachment 122678 [details] [diff] [review]
the fix
(Assignee)

Comment 2

15 years ago
Comment on attachment 122678 [details] [diff] [review]
the fix

hitting up caillon for review.
Attachment #122678 - Flags: review?(caillon)
(Assignee)

Comment 3

15 years ago
Created attachment 123381 [details] [diff] [review]
diff -y

much more readable diff -y

to keep reviewers friendly.
(Assignee)

Updated

15 years ago
Attachment #122678 - Flags: review?(caillon)
(Assignee)

Updated

15 years ago
Attachment #123381 - Flags: review?(caillon)
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;
Attachment #122678 - Flags: review+
(Assignee)

Comment 5

15 years ago
Created attachment 124807 [details] [diff] [review]
v2
Attachment #122678 - Attachment is obsolete: true
Attachment #123381 - Attachment is obsolete: true
(Assignee)

Comment 6

15 years ago
Created attachment 124808 [details] [diff] [review]
v2 diff -yw
(Assignee)

Updated

15 years ago
Attachment #123381 - Flags: review?(caillon)
(Assignee)

Comment 7

15 years ago
Created attachment 124809 [details] [diff] [review]
v2.1

uh no... the previous one was wrong. sorry for spam :/
Attachment #124807 - Attachment is obsolete: true
Attachment #124808 - Attachment is obsolete: true
(Assignee)

Comment 8

15 years ago
Created attachment 124810 [details] [diff] [review]
v2.1 diff -y
(Assignee)

Comment 9

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

carrying over r=caillon, requesting sr.

addresses caillon's comments.
Attachment #124809 - Flags: superreview?(dmose)
Attachment #124809 - Flags: review+
(Assignee)

Comment 10

14 years ago
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 ;)
Attachment #124809 - Flags: superreview?(dmose) → superreview?(bienvenu)

Comment 11

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

sr=bienvenu
Attachment #124809 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 12

14 years ago
thx!

fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Verified, lxr-style.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.