70576 - excessive calls to column.get() could be cleaned up

Status: RESOLVED INVALID

(MailNews Core :: Address Book, defect)

Description

[timeless]

there's no need to call .get() more than once for that for loop.

Comment 1

[Denis Antrushin]

INVALID ?

Comment 2

[timeless]

yes no one looked at this bug, not very nice of them.

i suspect that we could save code size by calling column.get() before the 
switch.  Can I please get some attention on this bug?

Comment 3

[Denis Antrushin]

After second look at the code, I think we can use valueSlot instead of
colunm.get() and typeSlot instead of colType.get() :-)

Comment 4

[Denis Antrushin]

Attachment: patch

Something like that.
Not sure if it's worth trouble though ;-)
But if it does, we can get rid of column at all, it's used only for
   column.ToLowerCase();
   column.Find("true");
which can be replaced with 
   PL_strcasecpm(valueSlot);

Comment 5

[(not reading, please use [e-mail] instead)]

taking, since chuang is gone.

this is a minor bug, pushing out.

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 67080 [details] [diff] [review]
patch

This is good cleanup, but there's more meat here than just the few column.get()
changes.

We should also:

 * lose column and colType, as you suggest

 * see if we should _really_ be using Find, rather than a simple strcmp

 * fix/remove the comment about duplication, since it seems to refer to itself
as the duplicate.  (Or, alternately, fix the dup function as well.)

Can we have another patch?

Comment 7

[Mike Shaver (:shaver emeritus)]

Re-assigning to timeless for a new patch, and marking with "perf" just for kicks.

(This doesn't need to be on Seth's buglist, and it won't get attention if it is.)

Comment 8

[timeless]

http://lxr.mozilla.org/seamonkey/ident?i=AddLdifColToDatabase


 * lose column and colType, as you suggest
Agreed.
 * see if we should _really_ be using Find, rather than a simple strcmp
strcmp looks right to me.
 * fix/remove the comment about duplication, since it seems to refer to itself
as the duplicate.  (Or, alternately, fix the dup function as well.)

AddLdifColToDatabase defined as a function in:
  mailnews/addrbook/src/nsAddressBook.cpp, line 848, as member of class 
AddressBookParser 
  mailnews/import/text/src/nsTextAddress.cpp, line 998, as member of class 
nsTextAddress 

Well, I can at least change the comment to point to the right other file. That 
function doesn't look anything like this one. New patch coming...

Comment 9

[timeless]

Attachment: first pass

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 68856 [details] [diff] [review]
first pass

>Index: nsAddressBook.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsAddressBook.cpp,v
>retrieving revision 1.81
>diff -u -r1.81 nsAddressBook.cpp
>--- nsAddressBook.cpp	6 Feb 2002 22:50:25 -0000	1.81
>+++ nsAddressBook.cpp	11 Feb 2002 12:48:52 -0000
>@@ -824,7 +824,7 @@
>     int length = 0;  // the length  of an ldif attribute
>     while ( (line = str_getline(&cursor)) != NULL)
>     {
>-        if ( str_parse_line(line, &typeSlot, &valueSlot, &length) == 0 )
>+        if ( str_parse_line(line, &typeSlot, &valueSlot, &length) == 0)

Skip this.  (I don't want to argue about whether it's consistent or not, just
skip it.)

>+// We have two copies of this function in the code, one here for migrating and 
>+// the other one in import/text/src/nsTextAddress.cpp for importing.  If ths 
>+// function need modification, make sure to change both places until we resolve

"function need_s_ modification".

> // this problem.
> void AddressBookParser::AddLdifColToDatabase(nsIMdbRow* newRow, char* typeSlot, char* valueSlot, PRBool bIsList)
> {
>-    nsCAutoString colType(typeSlot);
>-    nsCAutoString column(valueSlot);
>+  char colFirstChar=*typeSlot++;
>+  // Split the string into the first character and the rest of the string.

Preserve the 4-space indentation, puh-lease.  Parens around (*typeSlot) might
help clarity for future readers.  (Lots of people are iffy on precedence, and
I'm starting to see the value in coddling them a bit.)

>+  // 

Lose it.

>+  switch (colFirstChar)
>+  {
>+  case 'b':
>+    if (!strcmp(typeSlot, "irthyear"))

This is correct, but wants for clarity.  I don't think it's worth that clarity
cost to avoid retesting the leading character.	(See below where you don't
common-out the "ustom" pseudo-prefix either.)

What do you think of losing colFirstChar, and just doing

switch(typeSlot[0])
{
case 'b':
    if (!strcmp(typeSlot, "birthyear"))

instead?

If you really want the perf win (is this a high-temperature call?), then
you should look at something like gperf to generate a perfect hash and then
dispatch.

>+#if 0
>+    else if (!strcmp(typeSlot, "hoto"))
>+      ioRow->AddColumn(ev, this->ColPhoto(), yarn);
>+    else if (!strcmp(typeSlot, "arentphone"))
>+      ioRow->AddColumn(ev, this->ColParentPhone(), yarn);
>+    else if (!strcmp(typeSlot, "ageremail"))
>+      ioRow->AddColumn(ev, this->ColPagerEmail(), yarn);
>+    else if (!strcmp(typeSlot, "refurl"))
>+      ioRow->AddColumn(ev, this->ColPrefUrl(), yarn);
>+    else if (!strcmp(typeSlot, "riority"))
>+      ioRow->AddColumn(ev, this->ColPriority(), yarn);
>+#endif

Lose the #if 0'd blocks, please.  We have CVS, we might as well use it.

diff really did you no favours here, it seems.	For the next revision,
can you attach a copy of the new function in its patched form?	Just slam it
on the end of your diff or something.

Comment 11

[timeless]

> Skip this.
ok.
>>+// the other one in import/text/src/nsTextAddress.cpp for importing.  If ths 
th*e*
> "function need_s_ modification".
ok

> Preserve the 4-space indentation, puh-lease. 
It wasn't really 4 space. I reformatted it just to get some consistency, now 
it's easy to s/  /    / -- will do.

> Parens around (*typeSlot) might help clarity for future readers. 
If you insist, although I worry that doing that might confuse more readers. 
Although this is moot if I do what you suggest below.
> (Lots of people are iffy on precedence,
> and I'm starting to see the value in coddling them a bit.)

>>+  // 
> Lose it.
sure

>>+    if (!strcmp(typeSlot, "irthyear"))
> This is correct, but wants for clarity. 
+  // Split the string into the first character and the rest of the string.
I guess it wasn't good enough *sigh*

would |if (!strcmp(typeSlot, /*b*/"irthyear"))| be clearer to show what 
character(s) are already parsed?

> I don't think it's worth that clarity cost to avoid retesting the leading
> character. 

> (See below where you don't common-out the "ustom" pseudo-prefix either.)
? :o now i get it.  Yeah I knew that the common prefix stuck out as ugly but I 
wasn't trying to address everything at once, it was a trial balloon.  Do you 
think a helperfunction whose name / comment indicated that it advanced on match 
would be helpful?

if (MATCHES_PREFIX_WILLADVANCE_POINTER(typeSlot, "ustom"))
  switch(typeSlot) {
    case '1':
 ...

My guess is you'll veto this :-)

> What do you think of losing colFirstChar, and just doing
In fact I don't think I need the colFirstChar either way, it was only there 
because I was afraid of scaring people by doing |switch((*typeSlot)++)|
> switch(typeSlot[0])
yeah i debated between typeSlot[0] and *typeSlot and then realized that I could 
trial balloon saving comparisons.

> Lose the #if 0'd blocks, please.  We have CVS, we might as well use it.
I only put them there because my code changed the string comparisons.  And some 
of the function calls were dated.  I would happily check them in and then 
remove them for the next revision.  If we don't trim characters then I'd just 
remove the commented blocks.

> diff really did you no favours here, it seems. 
Indeed

> For the next revision,
> can you attach a copy of the new function in its patched form? 
yeah, I should do that.

--

Just a thought about execution operations, let's make a very very incorrect 
assumption: the file being imported contains many many company entries and 
virtually nothing else.

That means we'd hit the custom/company code block a lot...
Assuming I took your hint and rewrote the code to check for ustom repeatedly 
there would be one if (!strcmp (typeSlot, /*c*/"ustom")) which would have cost 
else + if + cmp[u] + cmp[ompany].  If left alone, the cost is 
(else+if+cmp[cu])*4 + cmp[company].  Which saves say 10 instructions and 
probably makes it totally worthless :-)

However, perhaps the code would be more readable for the customN cases.

Comment 12

[Mike Shaver (:shaver emeritus)]

If you want to make the performance optimal, you want a perfect hash: see gperf.

Comment 13

[(not reading, please use [e-mail] instead)]

sorry for being silent on this for so long.  been busy with other bugs.

you won't have to twist my arm to believe that the addressbook code needs work, 
and that there are low hanging performance problems.

but patches like this always come with a cost:  regressions, testing, and 
review time.

can you summarize the expected performance gains from this patch.  If it were 
to only speed up LDIF import (an infrequent user action) by 1%, then you could 
expect this to bit rot a while, while we focus on other bugs.

all the mailnews patches are going through a small number of people, and 
addressbook patches go through an even smaller number.  I'm trying to make sure 
we focus on the important stuff.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Timeless, still working on this patch?

(I'm not sure if this bug is still a bug.. )

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 68856 [details] [diff] [review]
first pass

This file in the patch no longer exists:

mailnews/addrbook/src/nsAddressBook.cpp

Thus the patch has at least (partially) bitrotted.

Comment 16

[:aceman]

This is surely orphaned.

Comment 17

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 18

[Wayne Mery (:wsmwk)]

no longer needed? ref comment 15