124022 - LDAP addressbook searches need to ask the server for just the attributes we handle.

Status: VERIFIED FIXED

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

Description

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

LDAP addressbook performance is slow

I might as well log this, before someone else does.

we should compare to 4.x first.

Comment 1

[yulian chang]

Reassign QA contact to myself

Comment 2

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

it might be something specific to my LDAP server.

dmose suggests we might getting back ALL attributes, instead of the just the 
ones we need, from the LDAP server.

my ldap server has large certificate attributes, which if they are coming over 
the wire, would slow us down.

Comment 3

[John Marmion]

I believe that we do return all the attributes associated with an LDAP entry. We
then see if these attributes exist in our LDAP/Mork mapping and only then return
the values associated with the matched attribute.

Comment 4

[Michael Buckland]

Discussed in 2/27/02 Mail & News bug meeting. Please advise if this happens on
autocomplete.

Comment 5

[scottputterman]

Seth, does this happen on autocomplete, too?

Comment 6

[scottputterman]

If we get some data that shows that autocomplete is significantly slower then we
should renominate.  It would also be good to get some data on LDAP search in the
address book.

Comment 7

[[e-mail] (gone - use [e-mail] instead)]

I need to get on this.  Yulian, can you please give me instructions privately on
how to connect to our LDAP server so I can begin perf testing? 

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

scott/michael: this bug is only about the LDAP addressbook integration; the
autocomplete performance is fine.

Comment 9

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

ok, we are getting more back from the LDAP server than we need.

setting a breakpoint in MozillaLdapPropertyRelator::findMozillaPropertyFromLdap
(), I see a bunch of stuff that won't map.

the stack for this is:

MozillaLdapPropertyRelator::findMozillaPropertyFromLdap(const char * 
0x017c5fa8) line 203
MozillaLdapPropertyRelator::createCardPropertyFromLDAPMessage(nsILDAPMessage * 
0x01840f40, nsIAbCard * 0x049a8208, int * 0x0012fbe8) line 236 + 18 bytes
nsAbQueryLDAPMessageListener::OnLDAPMessageSearchEntry(nsILDAPMessage * 
0x01840f40, nsIAbDirectoryQueryResult * * 0x0012fcb4) line 358 + 25 bytes
nsAbQueryLDAPMessageListener::OnLDAPMessage(nsAbQueryLDAPMessageListener * 
const 0x01843f60, nsILDAPMessage * 0x01840f40) line 219 + 36 bytes
XPTC_InvokeByIndex(nsISupports * 0x01843f60, unsigned int 3, unsigned int 1, 
nsXPTCVariant * 0x01883040) line 106
EventHandler(PLEvent * 0x01845028) line 566 + 41 bytes
PL_HandleEvent(PLEvent * 0x01845028) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x01681ec8) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x002b08c4, unsigned int 49402, unsigned int 0, 
long 23600840) line 1071 + 9 bytes
USER32! 77e13eb0()
USER32! 77e1401a()
USER32! 77e192da()
nsAppShellService::Run(nsAppShellService * const 0x042906e0) line 309
main1(int 2, char * * 0x00304850, nsISupports * 0x00000000) line 1350 + 32 bytes
main(int 2, char * * 0x00304850) line 1698 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

right now, the code just silently ignores what it can't map.

const MozillaLdapPropertyRelation* property = findMozillaPropertyFromLdap (attrs
[i]);
if (!property)
  continue;

we should be asking the LDAP server for just the properties we can handle.

the rest is just wasted bandwidth, extra load on the server, and extra work for 
the the client (mozilla.)

this will affect perf, especially when you've got things like certs coming over 
a 28.8 line!

Comment 10

[John Marmion]

Seth, I don't understand your last sentence. My understanding is that we won't
have cert *values* coming over the line if we don't request them. Yes, we will
have attributes names but not values. We only request values from attributes
names that we match.

I can certainly see the merit of only requesting attributes that we can match.
But how do we determine what attributes we can match without asking the server?
If we don't ask the server, the alternative is to request a fixed set of
attributes we support. What overhead is involved in requesting a load of
attributes from servers who may only have a small subset of what we support. At
the moment the mapping will be determined by nsAbLDAPProperties.cpp. The
discussion around bug #118454 shows that people want to continue to extend that
mapping. 

Ultimately, I believe the best solution to this is to allow users to create
their own mapping. This issue was also raised by bug #126749.

Comment 11

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

I'm seeing:

+	ldapProperty	0x03d0fbc0 "usercertificate;binary"

come over the wire at MozillaLdapPropertyRelator::findMozillaPropertyFromLdap()

I've debugged, and I've verified that we are getting attributes back from the 
server that we are just going to ignore.

I think we need to do something like:

Index: addrbook/src/nsAbLDAPDirectoryQuery.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp,v
retrieving revision 1.9
diff -u -w -r1.9 nsAbLDAPDirectoryQuery.cpp
--- addrbook/src/nsAbLDAPDirectoryQuery.cpp     6 Mar 2002 07:47:31 -0000
1.9
+++ addrbook/src/nsAbLDAPDirectoryQuery.cpp     11 Mar 2002 19:54:04 -0000
@@ -680,7 +680,7 @@
             // Meta property
             // require all attributes
             //
-            returnAttributes = "";
+            returnAttributes = "cn,mail,o,telephonenumber,l,nickname";
             break;
         }

the key will be to make that list work with our mapping.  right now, our 
mapping is hard coded, so this shouldn't be hard.  it's gets a little trickier 
when we allow for user defined, external schemas.

Comment 12

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

let me bring this back in to 1.0, I think we want this, and the fix for 1.0 
should be simple.

Comment 13

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

I should add that I'm seeing a performance improvement with that one line fix.

here comes a patch for review...

Comment 14

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

Attachment: patch, only ask for what we know we can handle.

Comment 15

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

this patch will only ask for:

"givenname,sn,surname,cn,commonname,displayname,xmozillanickname,mail,xmozillase
condemail,telephonenumber,homephone,fax,facsimiletelephonenumber,pager,pagerphon
e,mobile,cellphone,carphone,postofficebox,streetaddress,l,locality,st,region,pos
tal"

we could get extra fancy, and ask for just the visible columns, but that would 
take more work, and would involve fixing the card code, as well.

dmose, review?

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 73552 [details] [diff] [review]
patch, only ask for what we know we can handle.

One suggestion: count down to 0 instead of up from 0, since many processors
have "branch-zero" and "branch-not-zero" instructions to optimize for this
case.

r=dmose

Comment 17

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

Attachment: optimized for the if != 0 check, ala dmose.

correction, here is what we get over the wire:

modifytimestamp,xmozillausehtmlmail,description,notes,custom4,custom3,custom2,custom1,birthyear,homeurl,

workurl,countryname,company,o,departmentnumber,department,orgunit,ou,title,countryname,zip,postalcode,

region,st,locality,l,streetaddress,postofficebox,carphone,cellphone,mobile,pagerphone,pager,

facsimiletelephonenumber,fax,homephone,telephonenumber,
xmozillasecondemail,mail,xmozillanickname,displayname,commonname,cn,surname,sn,givenname

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 73554 [details] [diff] [review]
optimized for the if != 0 check, ala dmose.

r=dmose

Comment 19

[David :Bienvenu]

Comment on attachment 73554 [details] [diff] [review]
optimized for the if != 0 check, ala dmose.

sr=bienvenu

Comment 20

[Mike Shaver (:shaver emeritus)]

Comment on attachment 73554 [details] [diff] [review]
optimized for the if != 0 check, ala dmose.

a=shaver for 1.0 trunk checkin.

Comment 21

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

fixed.

Comment 22

[[e-mail] (gone - use [e-mail] instead)]

Can I put a printf or 2 in an LDAP file to verify what we get back from the
server, so I can verify this?

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

I think the NSPR logging module named 'ldap' will show you this info.

Comment 24

[[e-mail] (gone - use [e-mail] instead)]

I would expect to see the global 
MozillaLdapPropertyRelator::GetAllSupportedLDAPAttributes called, but I see:

396[34e0e88]: nsLDAPConnection::Run() entered
0[274ba8]: pending operation added; total pending operations now = 1
396[34e0e88]: InvokeMessageCallback entered
0[274ba8]: nsLDAPOperation::SearchExt(): called with aBaseDn 
= 'ou=people,dc=netscape,dc=com'; aFilter = '(|(mail=*jason*)(cn=*jason*)
(givenname=*jason*)(sn=*jason*))', aAttrCounts = 46, aSizeLimit = 100
0[274ba8]: pending operation added; total pending operations now = 2
396[34e0e88]: pending operation removed; total pending operations now = 1
396[34e0e88]: InvokeMessageCallback entered
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr = 'modifytimestamp'
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr = 'departmentnumber'
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr = 'title'
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr 
= 'facsimiletelephonenumber'
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr = 'telephonenumber'
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr = 'mail'
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr = 'commonname'
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr = 'surname'
0[274ba8]: nsLDAPMessage::GetValues(): called with aAttr = 'givenname'
396[34e0e88]: InvokeMessageCallback entered

when typing "jason" into the search window.

Is this good enough to verify?

Comment 25

[[e-mail] (gone - use [e-mail] instead)]

I've been enlightened.  Thanks for the explanation, Seth.  This makes sense, 
our server doesn't support all of the possible attributes, and these are the 
ones it returns.

Verified, current trunk builds (2002-04-08)

Comment 26

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

right, if this bug was still there, you'd be getting back a bunch more 
attributes (like passwd, vacation text, manager, etc...) that the LDAP server 
you are testing against supports

Comment 27

[Kristof Petr]

Add to #17 comment by Seth:

Query for 'countryname' attribute on wire is dubbled, because there is same
line of code twicely.

nsAbLDAPProperties.cpp (v. 1.9) lines 134 and 151.


Not sure to reopen this bug or not. But if someone want to fix it,
can he modified it the next way, please? It will make AB work better
on some ldap environments, like my ;-)



@ 143 line:
-     {MozillaProperty_String, "WorkCountry",        "countryname"},
+     {MozillaProperty_String, "WorkCountry",        "c"},


@ 120 line:
-     {MozillaProperty_String, "WorkAddress",        "streetaddress"},
+     {MozillaProperty_String, "WorkAddress",        "streetaddress"},
+     {MozillaProperty_String, "WorkAddress",        "street"},

Comment 28

[[e-mail] (gone - use [e-mail] instead)]

Kristof, please file that separately, thanks!

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Actually, that problem is already filed as bug 139786.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

I take that back.  bug 139786 is related but different.  So it would be worth
filing a bug on that.