190539 - Speed optimizations for OS/2

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, defect)

Description

[jhp (no longer active)]

By creating the import library from the DLL rather than the DEF file, it will
use ordinals rather than names for the exports.  This results in a loading
speedup of around 25%.

Comment 1

[jhp (no longer active)]

Attachment: patch

Comment 2

[Mark Smith [:mcs]]

Interesting. Is the behavior different on Microsoft Windows? I was under the
impression that if ordinals are included in a .DEF file they are favored over
names when creating the inport file.

Comment 3

[jhp (no longer active)]

The issue was that since we were creating the import library from the DEF file,
it was only exporting by name.  Now, with this patch, it exports by ordinals. 
We still use the DEF file to create the DLL.

Comment 4

[Mark Smith [:mcs]]

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

The patch looks fine. It is worth noting that for other platforms the list of
exported symbols (on Windows, the .DEF file) is created from files under
mozilla/directory/c-sdk/ldap/libraries, e.g., libldap.ex. That way, only the
officially supported functions are exported and ordinals are assigned
"manually" so that they will not change from one build to the next.

Comment 5

[Mark Smith [:mcs]]

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

I forgot to mention: please add a comment near the IMPLIB commands in the
Makefiles to say why things are done this way on OS/2, e.g.,

# Create import library from the DLL so that ordinals are used (25% faster load
time)

Comment 6

[jhp (no longer active)]

Attachment: patch v2

I had no idea about the *.ex files, so I decided to go that route.  Look it
over since I did make some changes that affect cross platform code. 
Specifically, I change the libprldap/Makefile.in GENEXPORTS step to be like
that in libldap/Makefile.in;  this just seems like the correct way to do it. 
Also, in genexports.pl, I need a way to prevent the comments from being written
for the OS/2 file, since the linker does not like those comments.  What I put
in there isn't pretty, but I was hoping you could help me with a better way to
write it.

Comment 7

[jhp (no longer active)]

Attachment: patch v2.1

Added comment per mcs' comment.

Comment 8

[Mark Smith [:mcs]]

Comment on attachment 115175 [details] [diff] [review]
patch v2.1

Are the changes to mozilla/directory/c-sdk/config/rules.mk still needed? Won't
the other changes take care of creating a .def file that includes ordinals on
OS/2?

Everything else looks fine to me.

Comment 9

[Mark Smith [:mcs]]

Comment on attachment 115175 [details] [diff] [review]
patch v2.1

Forgot to set review status.

Comment 10

[jhp (no longer active)]

Yes, the changes in rules.mk are still necessary.  While the DEF file is used to
create the DLL, I still prefer to create the import library directly from the DLL.

Comment 11

[Mark Smith [:mcs]]

Comment on attachment 115175 [details] [diff] [review]
patch v2.1

OK. This has my OK now. If you just want to land on the trunk, you can do so
(because the LDAP C SDK trunk is not used by Mozilla client builds). But to get
this checked into ldapcsdk_50_client_branch we need the usual sr and approvals.

Comment 12

[jhp (no longer active)]

Attachment: patch v2.2

OK, I was wrong about the rules.mk changes.  We do need to create the import
lib from the DEF file.	We are moving to the GCC compiler soon, and the tools
for that compiler do not allow the creation of import libs from DLLs, only from
DEF files.

Comment 13

[Mark Smith [:mcs]]

Comment on attachment 116104 [details] [diff] [review]
patch v2.2

OK.

Comment 14

[jhp (no longer active)]

Comment on attachment 116104 [details] [diff] [review]
patch v2.2

Looking for sr= in order to get this checked in to client branch.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 116104 [details] [diff] [review]
patch v2.2

sr=dmose; be sure this gets landed on both branch and trunk.

Comment 16

[Mike Kaply [:mkaply]]

Checked into branch.

That patch didn't apply to the LDAP trunk - I'll work with Javier to get that
resolved today.

Comment 17

[Mike Kaply [:mkaply]]

Attachment: FYI: patch against the trunk

For posterity, this is the LDAP trunk patch.

Comment 18

[Mike Kaply [:mkaply]]

I can't check into the LDAP trunk. Someone else want to or give me authority?

Comment 19

[Mark Smith [:mcs]]

Michael, I will commit this to the trunk very soon.

Comment 20

[Frankie]

Spam for bug 129472

Comment 21

[Mark Smith [:mcs]]

Committed to the trunk now also:

mozilla/directory/c-sdk/build.mk
  new revision: 5.8; previous revision: 5.7
mozilla/directory/c-sdk/configure.in
  new revision: 5.10; previous revision: 5.9
mozilla/directory/c-sdk/config/rules.mk
  new revision: 5.4; previous revision: 5.3
mozilla/directory/c-sdk/ldap/build/genexports.pl
  new revision: 5.1; previous revision: 5.0
mozilla/directory/c-sdk/ldap/libraries/libldap/Makefile.in
  new revision: 5.9; previous revision: 5.8
mozilla/directory/c-sdk/ldap/libraries/libprldap/Makefile.in
  new revision: 5.5; previous revision: 5.4
    Fix 190539 - Speed optimizations for OS/2.
        Changes merged from ldapcsdk_50_client_branch.