split OTS out of libxul on Windows to relieve pressure on the linker

RESOLVED FIXED in mozilla11

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
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!
Attachment #581978 - Flags: review?(khuey)
(Assignee)

Updated

6 years ago
Component: Layout: Text → Build Config
QA Contact: layout.fonts-and-text → build-config
(Assignee)

Comment 2

6 years ago
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.
Attachment #581978 - Attachment is obsolete: true
Attachment #581978 - Flags: review?(khuey)
Attachment #582015 - Flags: review?(khuey)
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.
Attachment #582015 - Flags: review?(khuey)
(Assignee)

Comment 4

6 years ago
Created attachment 582547 [details] [diff] [review]
patch v3, move OTS from libxul to gkmedias.dll on Windows
Assignee: nobody → jfkthame
Attachment #582015 - Attachment is obsolete: true
Attachment #582547 - Flags: review?(khuey)
(Assignee)

Comment 5

6 years ago
Tryserver run at https://tbpl.mozilla.org/?tree=Try&rev=751bb19ee5f3 looks OK (just some routine random-orange).
(Assignee)

Comment 6

6 years ago
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
Attachment #582547 - Flags: review?(khuey) → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c7652a33bf
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/f8c7652a33bf
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.