The purpose of the variable is to inverse declarations for most static libraries, by defining in the moz.build that defines them where they end up linked. That removes the burden to find the right place to add a SHARED_LIBRARY_LIBS, place the right ifdefs (and keep them in sync with moz.build DIRS changes), etc. I don't think this is how we want things to be defined in the end game, but it's a start, and i don't want to be blocked by bikeshedding and scope-creep (going for full fledged library definitions is going to be much more work, and that would mean consequent build time improvements would have to wait, and /that/ would be a shame).

This will be used to declare in what shared library or intermediate static library objects are going to be linked into.

Comment on attachment 828533 [details] [diff] [review] PoC for i18n, necko, uconv and xpcom_core This is what the change looks like for i18n, necko, uconv and xpcom_core. I'll do the rest. Further down the road we can remove some of those useless intermediates. Even further down the road we can remove the leaf fake libraries. This can also probably be simplified by the use of export(), now that we have that.

We're not currently using it anywhere, and this will made useless by FINAL_LIBRARY (except for a few things, but even then, it will have to be something else than a passthrough)

Parts were missing.

Revised to use lowercase for LIBRARY_NAME matching, so that XUL vs xul can be handled. This is kind of awful, but the alternative (making LIBRARY_NAME 'xul' on mac) is not less awful.

Sigh, this is a version that still does lower(), but not on _libs_by_objdir because otherwise the build breaks. I'm not sure if lower() is not worse than the alternative, which, fwiw looks like this: https://hg.mozilla.org/try/rev/bda92d9c041e

This is work in progress. This is what it looks like with almost everything using FINAL_LIBRARY. It builds on Linux, and I fixed the first round of errors I got on other architectures. Hopefully try will succeed or there'll be another round.

Note that as a side effect this removes almost all EXPORT_LIBRARY references (only about 10 left)

Comment on attachment 828612 [details] [diff] [review] Add a FINAL_LIBRARY variable to use in moz.build Review of attachment 828612 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +356,5 @@ > backend_file.xpt_name = '%s.xpt' % obj.module > elif isinstance(obj, VariablePassthru): > + if 'LIBRARY_NAME' in obj.variables: > + name = obj.variables['LIBRARY_NAME'] > + self._libs[name.lower()] = obj.objdir What's the significance of last write wins here? Can't we just derive these entries from _libs_by_objdir? @@ +357,5 @@ > elif isinstance(obj, VariablePassthru): > + if 'LIBRARY_NAME' in obj.variables: > + name = obj.variables['LIBRARY_NAME'] > + self._libs[name.lower()] = obj.objdir > + self._libs_by_objdir[obj.objdir] = name I don't care for extracting LIBRARY_NAME out of VariablePassthru like this. Could you convert it to a full-blown class, please? @@ +667,5 @@ > + backend_file = self._backend_files[objdir] > + if basename not in self._libs: > + raise Exception('No LIBRARY_NAME in the tree matches ' > + 'FINAL_LIBRARY from %s (%s)' % > + (backend_file.srcdir, basename)) This feels like something we should do in emitter since the logic is conceptually shared across backends.

Comment on attachment 828533 [details] [diff] [review] PoC for i18n, necko, uconv and xpcom_core Review of attachment 828533 [details] [diff] [review]: ----------------------------------------------------------------- This is beautiful.

Comment on attachment 828634 [details] [diff] [review] WiP Review of attachment 828634 [details] [diff] [review]: ----------------------------------------------------------------- I will continue giving +1's on this bug until it lands :)

This will be used to declare in what shared library or intermediate static library objects are going to be linked into.

This currently fails in multiple ways. - on windows debug builds, it fails to link, because of what i think is a serious bug we already have, it's just hidden. Will file it. - on linux pgo builds, it times-out when running the profileserver, which i can't reproduce locally :(

Refreshed against bug 937526

Refreshed against bug 935980 and bug 937526.

FWIW, this is what i got out of try talos for PGO performance: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=87b26c259eb9&newRev=bfce10eb4682&submit=true http://perf.snarkfest.net/compare-talos/index.html?oldRevs=33f5a234edc3&newRev=0a6d92d0eb7f&submit=true Apparently, there's a big win on dromaeo css, and no significant change on other results (although at the time i'm writing this there's not enough data for mac) Interestingly, linker max vsize got down 1.5MB, too, which is not a lot, but it's also better than what tbsaunde had got in a previous attempt with something different with the same kind of effect of getting rid of most SHARED_LIBRARY_LIBS, where memory usage went up significantly.

Actually rebased against bug 937526.

Rebased again. Sorry for the noise. This should be the last.

Comment on attachment 830449 [details] [diff] [review] Add a FINAL_LIBRARY variable to use in moz.build Review of attachment 830449 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/data.py @@ +327,5 @@ > +class LibraryDefinition(SandboxDerived): > + """Partial definition for a library""" > + __slots__ = ( > + 'basename', > + 'static_libraries', Please document static_libraries. @@ +337,5 @@ > + self.basename = basename > + self.static_libraries = [] > + > + def link_static_lib(self, sandbox, basename): > + self.static_libraries.append((sandbox['RELATIVEDIR'], basename)) I'm not sure why you need a full method for a one-liner only called once. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +296,5 @@ > + raise SandboxValidationError('FINAL_LIBRARY requires LIBRARY_NAME') > + if not sandbox.get('FORCE_STATIC_LIB') and \ > + not sandbox.get('LIBXUL_LIBRARY'): > + raise SandboxValidationError('FINAL_LIBRARY requires FORCE_STATIC_LIB or LIBXUL_LIBRARY') > + self._final_libs.append((sandbox, libname, final_lib)) Must you hold a reference to the sandbox? That will prevent gc. But we probably don't care given our scale.

Comment on attachment 831388 [details] [diff] [review] Use FINAL_LIBRARY for all (fake) libraries that end up linked in a single other library. Review of attachment 831388 [details] [diff] [review]: ----------------------------------------------------------------- Some of my favorite patches are ones that remove move lines than they add while accomplishing the same thing. This patch ranks high in that index!

(In reply to Gregory Szorc [:gps] from comment #23) > I'm not sure why you need a full method for a one-liner only called once. You tell me :) It just seemed to be the pattern. > ::: python/mozbuild/mozbuild/frontend/emitter.py > @@ +296,5 @@ > > + raise SandboxValidationError('FINAL_LIBRARY requires LIBRARY_NAME') > > + if not sandbox.get('FORCE_STATIC_LIB') and \ > > + not sandbox.get('LIBXUL_LIBRARY'): > > + raise SandboxValidationError('FINAL_LIBRARY requires FORCE_STATIC_LIB or LIBXUL_LIBRARY') > > + self._final_libs.append((sandbox, libname, final_lib)) > > Must you hold a reference to the sandbox? Yes, because the sandbox is used at consume_finished time. And there will be more of those kind of things in the future, fwiw.

As it turns out, I'm not going to use the sandbox as it was stored in that patch, so I'm just switching to using the relative directory directly. Future patches for upcoming bugs *will* store the sandboxes, but somewhere else.

Comment on attachment 831388 [details] [diff] [review] Use FINAL_LIBRARY for all (fake) libraries that end up linked in a single other library. Review of attachment 831388 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/build/Makefile.in @@ -17,5 @@ > -I$(srcdir)/../lwbrk/src \ > -I$(srcdir)/../unicharutil/src \ > -I$(srcdir)/../strres/src \ > -I$(srcdir)/../locale/src \ > -I$(srcdir)/../locale/src/$(LOCALE_DIR) \ LOCALE_DIR is still used here ::: toolkit/library/Makefile.in @@ +20,4 @@ > ifdef MOZ_CONTENT_SANDBOX > ifeq ($(OS_ARCH),WINNT) > LOCAL_INCLUDES += -I$(srcdir)/../sandboxbroker > SHARED_LIBRARY_LIBS += ../../security/sandbox/win/src/sandboxbroker/$(LIB_PREFIX)sandboxbroker.$(LIB_SUFFIX) How about this one? @@ +23,5 @@ > SHARED_LIBRARY_LIBS += ../../security/sandbox/win/src/sandboxbroker/$(LIB_PREFIX)sandboxbroker.$(LIB_SUFFIX) > endif > endif > > +# COMPONENT_LIBS is mosly useless since bug 935881, but is kept for mostly @@ +73,1 @@ > # component libraries No longer, I'd say

(In reply to :Ms2ger from comment #27) > Comment on attachment 831388 [details] [diff] [review] > Use FINAL_LIBRARY for all (fake) libraries that end up linked in a single > other library. > > Review of attachment 831388 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: intl/build/Makefile.in > @@ -17,5 @@ > > -I$(srcdir)/../lwbrk/src \ > > -I$(srcdir)/../unicharutil/src \ > > -I$(srcdir)/../strres/src \ > > -I$(srcdir)/../locale/src \ > > -I$(srcdir)/../locale/src/$(LOCALE_DIR) \ > > LOCALE_DIR is still used here Well, I guess I can remove that line. The headers in those directories are actually not included from anywhere else than those directories. So this -I was useless. > ::: toolkit/library/Makefile.in > @@ +20,4 @@ > > ifdef MOZ_CONTENT_SANDBOX > > ifeq ($(OS_ARCH),WINNT) > > LOCAL_INCLUDES += -I$(srcdir)/../sandboxbroker > > SHARED_LIBRARY_LIBS += ../../security/sandbox/win/src/sandboxbroker/$(LIB_PREFIX)sandboxbroker.$(LIB_SUFFIX) > > How about this one? It's linked in several places. This needs separate handling, which is next. > @@ +23,5 @@ > > SHARED_LIBRARY_LIBS += ../../security/sandbox/win/src/sandboxbroker/$(LIB_PREFIX)sandboxbroker.$(LIB_SUFFIX) > > endif > > endif > > > > +# COMPONENT_LIBS is mosly useless since bug 935881, but is kept for > > mostly > > @@ +73,1 @@ > > # component libraries > > No longer, I'd say COMPONENT_LIBS += $(MOZ_APP_COMPONENT_LIBS)

https://hg.mozilla.org/mozilla-central/rev/3d0fce244e6b https://hg.mozilla.org/mozilla-central/rev/a296036e608f https://hg.mozilla.org/mozilla-central/rev/7161a2d28363 https://hg.mozilla.org/mozilla-central/rev/8a50d16b1645