The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla11

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 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

5 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

5 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

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

Comment 2

5 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

5 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

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

Comment 6

5 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

5 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: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.