Last Comment Bug 711079 - split OTS out of libxul on Windows to relieve pressure on the linker
: split OTS out of libxul on Windows to relieve pressure on the linker
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-15 08:06 PST by Jonathan Kew (:jfkthame)
Modified: 2011-12-19 05:43 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, build OTS as separate library on Windows (8.23 KB, patch)
2011-12-15 08:18 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
patch v2, build OTS as separate library on Windows (8.23 KB, patch)
2011-12-15 09:53 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
patch v3, move OTS from libxul to gkmedias.dll on Windows (5.36 KB, patch)
2011-12-17 09:07 PST, Jonathan Kew (:jfkthame)
khuey: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2011-12-15 08:06:15 PST
Given the recent MSVC PGO headaches, I suggest that we build OTS (the opentype sanitizer library that we use to validate downloadable fonts) as a separate DLL instead of incorporating it into libxul.

It's a separate chunk of code with a simple API that we call, and it isn't a hot path, so any slight additional overhead of having a separate library should be unimportant. On an optimized Win32 build, libmozots.dll comes to around 130K ... not huge, but might help to buy us a little more headroom on the builders.
Comment 1 Jonathan Kew (:jfkthame) 2011-12-15 08:18:20 PST
Created attachment 581978 [details] [diff] [review]
patch, build OTS as separate library on Windows

Here's my attempt to split out OTS on Windows. As I don't really know much about the innards of our build system, it's possible I'm doing things in the wrong way or place....please point me in the right direction if necessary!
Comment 2 Jonathan Kew (:jfkthame) 2011-12-15 09:53:14 PST
Created attachment 582015 [details] [diff] [review]
patch v2, build OTS as separate library on Windows

Well, that didn't quite work, but I think this is looking better... tryserver run currently in progress at https://tbpl.mozilla.org/?tree=Try&rev=eff1d7eab6a7.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-16 06:35:04 PST
Comment on attachment 582015 [details] [diff] [review]
patch v2, build OTS as separate library on Windows

Review of attachment 582015 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good, but I think that we should put ots in gkmedia.dll.  You'll need to add $(MOZ_OTS_LIBS) to SHARED_LIBRARY_LIBS in layout/media to do that.

Clearing review cause I'd like to see it again before r+ing.

::: browser/installer/package-manifest.in
@@ +58,5 @@
>  @BINPATH@/@MOZ_CHILD_PROCESS_NAME@
>  #endif
>  #ifdef XP_WIN32
>  @BINPATH@/@DLL_PREFIX@gkmedias@DLL_SUFFIX@
> +@BINPATH@/@DLL_PREFIX@mozots@DLL_SUFFIX@

Which means we don't need this.

::: config/config.mk
@@ +148,5 @@
>  
>  MOZ_UNICHARUTIL_LIBS = $(LIBXUL_DIST)/lib/$(LIB_PREFIX)unicharutil_s.$(LIB_SUFFIX)
>  MOZ_WIDGET_SUPPORT_LIBS    = $(DIST)/lib/$(LIB_PREFIX)widgetsupport_s.$(LIB_SUFFIX)
>  
> +MOZ_OTS_LIB = $(DIST)/lib/$(LIB_PREFIX)mozots.$(LIB_SUFFIX)

I don't think you need this here.

::: configure.in
@@ -8207,5 @@
>  dnl ========================================================
> -dnl OTS
> -dnl ========================================================
> -MOZ_OTS_LIBS='$(DEPTH)/gfx/ots/src/$(LIB_PREFIX)mozots.$(LIB_SUFFIX)'
> -AC_SUBST(MOZ_OTS_LIBS)

You should probably keep this here.

::: gfx/ots/src/Makefile.in
@@ +33,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  MODULE         = ots
>  LIBRARY_NAME   = mozots
> +DIST_INSTALL   = 1

You don't need DIST_INSTALL here.

@@ +41,5 @@
> +VISIBILITY_FLAGS =
> +else
> +LIBXUL_LIBRARY   = 1
> +FORCE_STATIC_LIB = 1
> +endif

We can FORCE_STATIC_LIB in both cases, but we need LIBXUL_LIBRARY to be conditional on not being on Windows.

@@ +105,5 @@
> +else
> +LDFLAGS += $(MOZ_ZLIB_LIBS)
> +endif
> +
> +LDFLAGS += $(MOZALLOC_LIB)

Don't need this ZLIB or MOZALLOC stuff either, I think.

::: js/src/config/config.mk
@@ +148,5 @@
>  
>  MOZ_UNICHARUTIL_LIBS = $(LIBXUL_DIST)/lib/$(LIB_PREFIX)unicharutil_s.$(LIB_SUFFIX)
>  MOZ_WIDGET_SUPPORT_LIBS    = $(DIST)/lib/$(LIB_PREFIX)widgetsupport_s.$(LIB_SUFFIX)
>  
> +MOZ_OTS_LIB = $(DIST)/lib/$(LIB_PREFIX)mozots.$(LIB_SUFFIX)

I don't think you need this here either.

::: mobile/xul/installer/package-manifest.in
@@ +59,5 @@
>  @BINPATH@/@MOZ_CHILD_PROCESS_NAME@
>  #endif
>  #ifdef XP_WIN32
>  @BINPATH@/@DLL_PREFIX@gkmedias@DLL_SUFFIX@
> +@BINPATH@/@DLL_PREFIX@mozots@DLL_SUFFIX@

And we don't need this.

::: toolkit/library/Makefile.in
@@ +377,5 @@
>    $(MOZ_APP_EXTRA_LIBS) \
>    $(SQLITE_LIBS) \
>    $(NULL)
>  
> +EXTRA_DSO_LDOPTS += $(call EXPAND_LIBNAME_PATH,mozots,$(DIST)/lib)

Don't need this.
Comment 4 Jonathan Kew (:jfkthame) 2011-12-17 09:07:22 PST
Created attachment 582547 [details] [diff] [review]
patch v3, move OTS from libxul to gkmedias.dll on Windows
Comment 5 Jonathan Kew (:jfkthame) 2011-12-17 23:40:55 PST
Tryserver run at https://tbpl.mozilla.org/?tree=Try&rev=751bb19ee5f3 looks OK (just some routine random-orange).
Comment 6 Jonathan Kew (:jfkthame) 2011-12-17 23:57:12 PST
I also pushed a tryserver job with this patch, plus setting "MOZ_GRAPHITE=1" in configure.in and "mk_add_options MOZ_PGO=1" in mozconfig; as I understand it, this should give me a PGO Windows build.

This got 5 out of 5 green builds: https://tbpl.mozilla.org/?tree=Try&rev=74f41f99a7a4
Comment 8 Marco Bonardo [::mak] 2011-12-19 05:43:50 PST
https://hg.mozilla.org/mozilla-central/rev/f8c7652a33bf

Note You need to log in before you can comment on or make changes to this bug.