Closed Bug 207407 Opened 21 years ago Closed 21 years ago

Enable building a linkable library of gecko components

Categories

(SeaMonkey :: Build Config, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bryner, Assigned: bryner)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

Some platforms (Mac) can get a major startup time boost from being able to
prebind libraries, which requires having all of the symbols present at link
time.  I came up with an approach that allows us to do this by creating a
linkable library containing all (well, whichever ones you build) of the gecko
components, which can then be registered via the static component loader.  Your
application or embedding wrapper would link directly against this library.

I did some measurements on Mac OS X (10.2.6) and found that I got around 35%
startup time improvement using this approach with mozilla-bin.
Attached patch patch (obsolete) — Splinter Review
note: I didn't actually add the directory embedding/componentlib to CVS, in
case reviewers think another location would be better.	I just diff'd the files
there against /dev/null and appended it to the patch, hopefully it will apply.
Attached patch patchSplinter Review
oops, this wasn't supposed to include the embedding/browser/cocoa change.
Attachment #124421 - Attachment is obsolete: true
Attachment #124422 - Flags: review?(seawood)
Attachment #124422 - Flags: superreview?(sfraser)
Attachment #124422 - Flags: review?(ccarlen)
Comment on attachment 124422 [details] [diff] [review]
patch

>Index: browser/app/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/browser/app/Makefile.in,v

Is this part of this patch?

>Index: config/config.mk
>===================================================================
>RCS file: /cvsroot/mozilla/config/config.mk,v
>retrieving revision 3.270
>diff -u -r3.270 config.mk
>--- config/config.mk	16 May 2003 06:07:35 -0000	3.270
>+++ config/config.mk	29 May 2003 06:23:51 -0000
>@@ -695,6 +695,18 @@
> endif
> endif
> 
>+# Flags needed to link against the component library
>+ifdef MOZ_COMPONENTLIB
>+MOZ_COMPONENTLIB_EXTRA_DSO_LIBS = mozcomps xpcom_compat
>+
>+# Tell the linker where NSS is, if we're building crypto
>+ifeq ($(OS_ARCH),Darwin)
>+ifeq (,$(findstring crypto,$(MOZ_META_COMPONENTS)))
>+MOZ_COMPONENTLIB_EXTRA_LIBS = $(foreach library, $(patsubst -l%, $(LIB_PREFIX)%$(DLL_SUFFIX), $(filter -l%, $(NSS_LIBS))), -dylib_file @executable_path/$(library):$(DIST)/bin/$(library))
>+endif
>+endif
>+endif
>+
> #
> # Include any personal overrides the user might think are needed.
> #

I'm not convinced that the following doesn't belong in modules/staticmod. Maybe
 modules/staticmod should be renamed and generalized?

>--- /dev/null	Wed May 28 20:21:56 2003
>+++ embedding/componentlib/.cvsignore	Wed May 28 23:42:22 2003
>@@ -0,0 +1 @@
>+Makefile
>--- /dev/null	Wed May 28 20:21:56 2003
>+++ embedding/componentlib/Makefile.in	Wed May 28 15:47:41 2003
>+
>+# We have to invoke ld directly so that force_data_segment.o can be linked
>+# before dylib1.o
>+MKSHLIB = $(LD) $(DSO_LDOPTS) -o $@
>+

>+# force it to be first
>+SHLIB_LDSTARTFILE = force_data_segment.o -ldylib1.o
>+SHLIB_LDSTARTFILE = -ldylib1.o

Why is SHLIB_LDSTARTFILE here twice? Typo?

The rest looks good.
Comment on attachment 124422 [details] [diff] [review]
patch

>Index: config/rules.mk
>===================================================================
>+EXTRA_DSO_LDOPTS	+= -dynamiclib -install_name @executable_path/$(SHARED_LIBRARY) -compatibility_version 1 -current_version 1

Would it make sense to pull those verion numbers into separate make variables,
so that we can rev them when we make changes to the lib that break backwards
compatibility?


>Index: xpfe/bootstrap/nsAppRunner.cpp
>===================================================================

>+static nsStaticModuleInfo staticInfo[] = {
>+  {
>+    "meta component",
>+    nsMetaModule_nsGetModule
>+  }
>+};

Can this be const?

>+++ embedding/componentlib/Makefile.in	Wed May 28 15:47:41 2003

>+# A limitation exists in the Mach-O format which causes "scattered" relocation
>+# entries in shared libraries to be limited to a 24-bit address space (16 MB).
>+# The high 8 bits of the address are dropped.  This can cause problems in
>+# a mozcomps build, particularly non-optimized builds.  To avoid the problem,
>+# we move the data segment before the code segment in the libraray.  The data
>+# segment is relatively small, and all of the relocations in question
>+# (which are pointers to construction vtables) reside in the
>+# (__DATA,__const) section.
>+#
>+# See also the 5th item in the TODO section of:
>+#   http://www.opensource.apple.com/darwinsource/10.2.6/cctools/ld/notes
>+#  (free APSL registration required)
>+
>+

>+# XXX this causes radar bug 3268595
>+#EXTRA_DSO_LDOPTS += -exported_symbols_list exported_symbols

Say what radar bug 3268595 is about. Is it the 24-bit limitation described
above?

>+++ embedding/componentlib/nsMetaModule.cpp.in	Wed May 28 15:47:41 2003
>@@ -0,0 +1,120 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ *
>+ * The contents of this file are subject to the Netscape Public
>+ * License Version 1.1 (the "License"); you may not use this file

Should this be NPL, or the MPL tri-license?

Address comments, and sr=me
Attachment #124422 - Flags: superreview?(sfraser) → superreview+
> Would it make sense to pull those verion numbers into separate make variables,
> so that we can rev them when we make changes to the lib that break backwards
> compatibility?

Well, the only change to the library that could break backwards compatibility is
changing the signature of nsMetaModule_nsGetModule.  We've never actually used
-current_version and -compatibility_version correctly for Gecko, I don't think.
 It would definitely make sense to use these version numbers correctly for the
NSBrowserView framework.

> Can this be const?

Not without changing the definition of NSGetStaticModuleInfoFunc, since it has a
non-const nsStaticModuleInfo out-pointer.

> Say what radar bug 3268595 is about. Is it the 24-bit limitation described
> above?

No, it's not.  This bug (which was since duped against radar 3126383, which
means I can't follow it anymore) is about a situation where after prebinding is
fixed for a binary, some C++ static initializers don't run anymore.  I'll add a
comment about this in the source.

> Should this be NPL, or the MPL tri-license?

I copied it from modules/staticmod/nsMetaModule.cpp.in, so I didn't figure it
would be appropriate to change the license.
Comment on attachment 124422 [details] [diff] [review]
patch

r=ccarlen with the comment about SHLIB_LDSTARTFILE fixed.
Attachment #124422 - Flags: review?(ccarlen) → review+
checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: