Closed
Bug 70576
Opened 23 years ago
Closed 4 years ago
excessive calls to column.get() could be cleaned up
Categories
(MailNews Core :: Address Book, defect)
MailNews Core
Address Book
Tracking
(Not tracked)
RESOLVED
INVALID
People
(Reporter: timeless, Unassigned)
References
()
Details
(Keywords: perf, Whiteboard: [patchlove])
Attachments
(2 obsolete files)
there's no need to call .get() more than once for that for loop.
QA Contact: esther → fenella
Comment 1•23 years ago
|
||
INVALID ?
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?
Summary: excessive calls to object.get() should be cleaned up → excessive calls to column.get() could be cleaned up
Comment 3•22 years ago
|
||
After second look at the code, I think we can use valueSlot instead of colunm.get() and typeSlot instead of colType.get() :-)
Comment 4•22 years ago
|
||
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•22 years ago
|
||
taking, since chuang is gone. this is a minor bug, pushing out.
Assignee: chuang → sspitzer
Severity: normal → minor
Target Milestone: --- → mozilla1.0
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?
Attachment #67080 -
Flags: needs-work+
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.)
Assignee: sspitzer → timeless
Keywords: perf
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...
Status: NEW → ASSIGNED
Attachment #67080 -
Attachment is obsolete: true
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.
Attachment #68856 -
Flags: needs-work+
Reporter | ||
Comment 11•22 years ago
|
||
> 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.
If you want to make the performance optimal, you want a perfect hash: see gperf.
Comment 13•22 years ago
|
||
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.
Updated•22 years ago
|
Whiteboard: mailreviewtest
Updated•20 years ago
|
Product: Browser → Seamonkey
Updated•19 years ago
|
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
QA Contact: fenella → addressbook
Assignee | ||
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 14•15 years ago
|
||
Timeless, still working on this patch? (I'm not sure if this bug is still a bug.. )
Updated•15 years ago
|
Whiteboard: mailreviewtest → [patchlove]
Target Milestone: mozilla1.0 → ---
Comment 15•15 years ago
|
||
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.
Attachment #68856 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Updated•4 years ago
|
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(geoff)
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•