Enable building a linkable library of gecko components

RESOLVED FIXED

Status

SeaMonkey
Build Config
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Brian Ryner (not reading))

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

18.21 KB, patch
Conrad Carlen (not reading bugmail)
: review+
Simon Fraser
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
Created attachment 124421 [details] [diff] [review]
patch

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.
(Assignee)

Comment 2

15 years ago
Created attachment 124422 [details] [diff] [review]
patch

oops, this wasn't supposed to include the embedding/browser/cocoa change.
Attachment #124421 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124422 - Flags: review?(seawood)
(Assignee)

Updated

15 years ago
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 4

15 years ago
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+
(Assignee)

Comment 5

15 years ago
> 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+
(Assignee)

Comment 7

15 years ago
checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.