564851 - support building with profile-guided optimization (PGO)

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[(not currently active) Ted Mielczarek]

We're planning on building Firefox Linux builds with PGO, and if we also link NSPR in as a static library (bug 561842), it results in suboptimal behavior because GCC doesn't reorder functions and blocks in the NSPR code, meaning that we have a large block of code that foils our optimized shared library loading. It's fairly straightforward to add the necessary build machinery to NSPR's build system.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Add support for GCC and MSVC PGO (checked in to NSPR 4.8.5)

This patch is almost entirely copied from the top-level Mozilla build system. It adds support for GCC and MSVC's PGO compiler and linker flags. There's a configure test that checks for the GCC options, the MSVC ones are hardcoded in the win32/MSVC block.

Building using PGO is executed the same way as the Mozilla build system, by running:
make MOZ_PROFILE_GENERATE=1 (to generate instrumented binaries)
then a separate profiling step, for which there is no support included here, then
make clean (with GCC only, it needs to recompile all objects)
make MOZ_PROFILE_USE=1 (to generate optimized binaries)

Mozilla's client.mk has a "profiledbuild" target that takes care of all this for us. I think it's ok to make these manual steps for NSPR (as it is with SpiderMonkey), since there's no real client binaries being produced as part of the build. When building NSPR as part of the Firefox build, the PGO build will propogate down into the NSPR build system.

Comment 2

[Wan-Teh Chang]

Comment on attachment 444442 [details] [diff] [review]
Add support for GCC and MSVC PGO (checked in to NSPR 4.8.5)

r=wtc.  Thanks.

In config/config.mk:

>+# Enable profile-based feedback

Should this say "Enable profile-guided optimization"?

In configure.in:

>+if test $result = "yes"; then
>+  PROFILE_GEN_LDFLAGS="-fprofile-generate"
>+  PROFILE_USE_CFLAGS="-fprofile-use -fprofile-correction -Wcoverage-mismatch -freorder-blocks-and-partition"
>+  PROFILE_USE_LDFLAGS="-fprofile-use"
>+fi

Please indent by 4.

Comment 3

[(not currently active) Ted Mielczarek]

Committed to NSPR CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.276; previous revision: 1.275
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.280; previous revision: 1.279
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/nsprpub/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 1.40; previous revision: 1.39
done
Checking in config/config.mk;
/cvsroot/mozilla/nsprpub/config/config.mk,v  <--  config.mk
new revision: 3.32; previous revision: 3.31
done

Comment 4

[(not currently active) Ted Mielczarek]

I somehow typoed one of the compiler flags in my commit (even though it's correct in the patch here), I checked in a fix:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.278; previous revision: 1.277
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.282; previous revision: 1.281
done

Comment 6

[Tetsuro Kato [:tete]]

(In reply to comment #1)
> When building NSPR as part of the Firefox build, the PGO build will
> propogate down into the NSPR build system.

I think PGO hasn't been applied to NSPR DLLs.
Shouldn't we add PROFILE_GEN_LDFLAGS/PROFILE_USE_LDFLAGS to DLLFLAGS?


# /nsprpub/config/config.mk

 LDFLAGS		= $(OS_LDFLAGS)
 
 # Enable profile-guided optimization
 ifdef MOZ_PROFILE_GENERATE
 CFLAGS += $(PROFILE_GEN_CFLAGS)
 LDFLAGS += $(PROFILE_GEN_LDFLAGS)
+DLLFLAGS += $(PROFILE_GEN_LDFLAGS)
 endif # MOZ_PROFILE_GENERATE
 
 ifdef MOZ_PROFILE_USE
 CFLAGS += $(PROFILE_USE_CFLAGS)
 LDFLAGS += $(PROFILE_USE_LDFLAGS)
+DLLFLAGS += $(PROFILE_USE_LDFLAGS)
 endif # MOZ_PROFILE_USE
 
 define MAKE_OBJDIR
 if test ! -d $(@D); then rm -rf $(@D); $(NSINSTALL) -D $(@D); fi
 endef
 
 LINK_DLL	= $(LD) $(OS_DLLFLAGS) $(DLLFLAGS)

Comment 7

[(not currently active) Ted Mielczarek]

You are probably right. Can you attach your proposal as a patch?

Comment 8

[Tetsuro Kato [:tete]]

Attachment: Add PGO options to DLLFLAGS

Add PROFILE_GEN_LDFLAGS/PROFILE_USE_LDFLAGS to DLLFLAGS for NSPR DLLs.

Comment 9

[Tetsuro Kato [:tete]]

I'm afraid I'm not familiar with rules of Bugzilla. Is this okay?

Comment 10

[Tetsuro Kato [:tete]]

My patch is not enough.
It is necessary to add some rule to merge the pgc files into the pgd file and re-link the DLL file in the MOZ_PROFILE_USE phase.

Comment 11

[(not currently active) Ted Mielczarek]

Ah, there's some code in the top-level config/rules.mk for supporting that:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#970

If you'd like to copy that (and the pgomerge.py script) to NSPR, you can roll it up with your other patch. I think the problem here is that I only tested this with GCC's PGO, not Visual C++.

Comment 12

[Tetsuro Kato [:tete]]

Attachment: Add support for MSVC's PGO rv1.0

This patch adds the support of MSVC's PGO.
It adds a few rules and a script file (pgomerge.py) to merge the pgc files into the pgd file and re-link the DLL or program in the MOZ_PROFILE_USE phase.

The rules are created by reference to the following file:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk

The pgomerge.py is a copy of the following file:
http://mxr.mozilla.org/mozilla-central/source/build/win32/pgomerge.py

Comment 13

[Wan-Teh Chang]

Comment on attachment 463530 [details] [diff] [review]
Add support for MSVC's PGO rv1.0

r=wtc.  I suggest one change below.

In nsprpub/config/rules.mk:

>+################################################################################
>+
>+ifdef MOZ_PROFILE_USE
>+ifeq ($(NS_USE_GCC)_$(OS_ARCH),_WINNT)
>+# When building with PGO, we have to make sure to re-link
>+# in the MOZ_PROFILE_USE phase if we linked in the
>+# MOZ_PROFILE_GENERATE phase. We'll touch this pgo.relink
>+# file in the link rule in the GENERATE phase to indicate
>+# that we need a relink.
>+$(SHARED_LIBRARY): pgo.relink
>+
>+$(PROGRAM): pgo.relink
>+
>+endif	# WINNT && !GCC
>+endif	# MOZ_PROFILE_USE

Can you add this block of code elsewhere?
You added it in the middle of the "all, export,
libs, clean, clobber" rules, which seems out of
place.

I suggest adding the block of code either after
the $(SHARED_LIBRARY) rule (my preference) or
before the $(PROGRAM) rule:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/config/rules.mk&rev=3.78&mark=280-295,316-342#280

Comment 14

[Tetsuro Kato [:tete]]

Attachment: Add support for MSVC's PGO rv1.1

Added the block of code after the $(SHARED_LIBRARY) rule.

Comment 15

[Wan-Teh Chang]

Comment on attachment 494334 [details] [diff] [review]
Add support for MSVC's PGO rv1.1

r=wtc.

Nit: I would omit the
################################################################################
boundaries in nsprpub/config/rules.mk.

Comment 16

[Wan-Teh Chang]

Ted, can you check in the second patch in this bug?

Comment 17

[Steve Snyder]

The process of copying the *.PGC files from ~/dist/bin/ back to the build directory seems to be broken in Firefox 4.0 RC1 (Build #1).  Or at least it is with a Win64 build using VS2008/SP1 on a fully-updated WinXP x64 system.

When it comes time to re-link the instrumented builds I get output like this:

PGOMGR : warning PG0188: No .PGC files matching 'xul!*.pgc' were found.
xul.pgd(0) : warning C4961: No profile data was merged into 'xul.pgd', profile-guided optimizations disabled

Hmmm.  Maybe the *.PGC files were not actually generated?  Nope, there they are:

$ find obj-ff/ -name '*.pg?' | xargs ls -lt -hk
    67 Mar  8 12:56 obj-ff/memory/mozalloc/mozalloc.pgd
     3 Mar  8 12:56 obj-ff/dist/bin/firefox!2.pgc
     2 Mar  8 12:56 obj-ff/dist/bin/mozalloc!2.pgc
   788 Mar  8 12:56 obj-ff/dist/bin/mozsqlite3!2.pgc
     3 Mar  8 12:56 obj-ff/dist/bin/xpcom!2.pgc
 25376 Mar  8 12:56 obj-ff/dist/bin/xul!2.pgc
     3 Mar  8 12:47 obj-ff/dist/bin/firefox!1.pgc
     2 Mar  8 12:47 obj-ff/dist/bin/mozalloc!1.pgc
  1053 Mar  8 12:47 obj-ff/dist/bin/mozsqlite3!1.pgc
     3 Mar  8 12:47 obj-ff/dist/bin/xpcom!1.pgc
 25399 Mar  8 12:47 obj-ff/dist/bin/xul!1.pgc
    91 Mar  8 12:44 obj-ff/browser/app/firefox.pgd
   747 Mar  8 12:44 obj-ff/browser/components/build/browsercomps.pgd
   203 Mar  8 12:43 obj-ff/js/src/xpconnect/shell/xpcshell.pgd
    67 Mar  8 12:43 obj-ff/ipc/app/plugin-container.pgd
    83 Mar  8 12:43 obj-ff/xpcom/stub/xpcom.pgd
 78699 Mar  8 12:42 obj-ff/toolkit/library/xul.pgd
  1371 Mar  8 11:46 obj-ff/db/sqlite3/src/mozsqlite3.pgd
  7195 Mar  8 11:33 obj-ff/js/src/shell/js.pgd

Maybe the *.PGC files were actually merged and pgrmgr is lying about it's activity?  No, the PGD file is the same size and date as it it was before the profiling run.

Maybe the *.PGC files were not actually copied to the build directory?  Bingo!  There are no *PGC files in the build directories.  Re-running the linker in, say, /toolkit/library gets me the same complaint as above.  Manually copy the files:

   cp -p ../../dist/bin/xul*.pgc .

and all is well.  No complaints about missing profile data, and the linker explicitly states that the *.PGC files are being merged.

It appears that the process of copying the generated *.PGC files got broken.

Comment 18

[(not currently active) Ted Mielczarek]

Steve: that's kind of unrelated to this bug, which is just about PGO in NSPR. Regardless, the code for merging PGC files hasn't changed in ages:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#974

and you can see it working in the latest nightly build:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1299495847.1299523657.22417.gz&fulltext=1
d:/mozilla-build/python25/python2.5.exe /e/builds/moz2_slave/cen-w32-ntly/build/build/win32/pgomerge.py \
	  xul ../../dist/firefox
Microsoft (R) Profile Guided Optimization Manager 8.00.50727.762
Copyright (C) Microsoft Corporation. All rights reserved.

Merging ..\..\dist\firefox\xul!1.pgc

Now that I type that all out, I notice that we did change things in bug 589971. We're now running the profiling step on the staged package bits in dist/firefox, not dist/bin.

Comment 19

[Steve Snyder]

So I should be doing the instrumented run from ~/dist/firefox/ now?  I guess I need to edit my PGO script.

Sorry to clutter this bug.

Comment 20

[(not currently active) Ted Mielczarek]

Steve: yes, that's what you need to do. If you use the in-tree profileserver.py, it will run the binary from the correct place.

Sorry, I forgot to check this in until now:
RCS file: /cvsroot/mozilla/nsprpub/build/win32/pgomerge.py,v
done
Checking in build/win32/pgomerge.py;
/cvsroot/mozilla/nsprpub/build/win32/pgomerge.py,v  <--  pgomerge.py
initial revision: 1.1
done
Checking in config/config.mk;
/cvsroot/mozilla/nsprpub/config/config.mk,v  <--  config.mk
new revision: 3.34; previous revision: 3.33
done
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.80; previous revision: 3.79
done

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

pushed nspr patch as part of http://hg.mozilla.org/mozilla-central/rev/b20b6d500c05