support building with profile-guided optimization (PGO)

RESOLVED FIXED in 4.8.9

Status

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Blocks 1 bug)

other
4.8.9
x86
All
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
OS: Linux → All
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.
Attachment #444442 - Flags: review?(wtc)

Comment 2

9 years ago
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.
Attachment #444442 - Flags: review?(wtc) → review+
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8.5
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
Duplicate of this bug: 571874
(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)
You are probably right. Can you attach your proposal as a patch?
Posted patch Add PGO options to DLLFLAGS (obsolete) — Splinter Review
Add PROFILE_GEN_LDFLAGS/PROFILE_USE_LDFLAGS to DLLFLAGS for NSPR DLLs.
I'm afraid I'm not familiar with rules of Bugzilla. Is this okay?
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.
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++.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #462191 - Attachment is obsolete: true
Attachment #463530 - Flags: review?(wtc)

Comment 13

9 years ago
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
Attachment #463530 - Flags: review?(wtc) → review+
Added the block of code after the $(SHARED_LIBRARY) rule.
Attachment #463530 - Attachment is obsolete: true
Attachment #494334 - Flags: review?

Updated

9 years ago
Attachment #494334 - Flags: review? → review?(wtc)

Comment 15

9 years ago
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.
Attachment #494334 - Flags: review?(wtc) → review+

Updated

9 years ago
Attachment #444442 - Attachment description: Add support for GCC and MSVC PGO → Add support for GCC and MSVC PGO (checked in to NSPR 4.8.5)

Comment 16

9 years ago
Ted, can you check in the second patch in this bug?
Target Milestone: 4.8.5 → 4.8.8

Comment 17

8 years ago
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.
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

8 years ago
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.
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
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Target Milestone: 4.8.8 → 4.8.9
You need to log in before you can comment on or make changes to this bug.