659942 - Clean up profiling data during PROFILE_GENERATE phase

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Considering profiling data is cumulative, it would be safer to remove it when we build with MOZ_PROFILE_GENERATE.

I'm tempted to say this should be done in the export rule when MOZ_PROFILE_GENERATE=1.

Comment 1

[Justin Lebar (not reading bugmail)]

If I'm doing my analysis correctly (I'm not so confident), there's no statistical difference in the Dromaeo DOM scores between a builder's first PGO build and its later builds.  (I compared first PGO versus all later PGOs, first PGO versus second PGO, and first versus third and later PGOs.)

This is still the best theory we've got about bug 653961, and even if it doesn't solve the variance issue, we probably still want to do it.  Once we check it in, it should be clear pretty quickly whether it solves the variance issue.  (And let's hope that the performance gets pegged at the high end of the range!)

Comment 2

[Mike Hommey [:glandium]]

Attachment: Tentative patch

This needs testing.

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> Created attachment 535352 [details] [diff] [review] [review]
> Tentative patch
> 
> This needs testing.

This obviously doesn't remove the profiling data from e.g. nspr.

Comment 4

[Justin Lebar (not reading bugmail)]

(In reply to comment #3)
> This obviously doesn't remove the profiling data from e.g. nspr.

So should we just do

  find . -name '*.gcda' | xargs rm -f

?

It would be nice to check this in and see if it affects bug 653961.

Comment 5

[Justin Lebar (not reading bugmail)]

Based on my latest tests (bug 653961 comment 79) I think there's a good chance this will affect the variance we've been seeing.

Comment 6

[Justin Lebar (not reading bugmail)]

According to [1], on Windows the .pgd files are the result of linking the instrumented build, not running it.  So maybe we can just leave them alone and let them be overwritten during the first pass.

[1] http://msdn.microsoft.com/en-us/library/aa289170%28v=vs.71%29.aspx

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Like so?

Comment 8

[(not currently active) Ted Mielczarek]

No, the pgd files get generated by the linker in the first pass if they don't already exist (from your link, "The result of linking /LTCG:PGI will be an executable or DLL and a PGO database file (.PGD)."), and the profile data from the run gets merged into them in the second pass.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 538407 [details] [diff] [review]
Patch v2

Okay, how about this?

Comment 11

[Justin Lebar (not reading bugmail)]

I think I'd prefer to land this change a few days ahead of bug 663251.  That way, we can see how the variance changes first with this patch and then with that patch.

Comment 12

[(not currently active) Ted Mielczarek]

I actually think I'd prefer glandium's approach. I don't think NSPR is a big deal, we can fix that separately. Doing a separate find is kind of gross.

Comment 13

[Justin Lebar (not reading bugmail)]

> I don't think NSPR is a big deal

It seems that very small differences in profiling data cause significant differences in benchmark scores, so I wouldn't write off the possibility of this mattering.

I don't care how we do it so long as we're reasonably sure that we're getting rid of all the files now and in the future.  I'll run a build with Glandium's patch to see if it's just NSPR we're missing, although the fact that I have to do this test (and don't have the ability to do it on Windows) is a large part of why I might prefer using find and xargs.

Comment 14

[Justin Lebar (not reading bugmail)]

I just tested Glandium's patch, and it appears that nsprpub is the only folder which keeps .gcda files.  Using find and xargs still seems safer to me -- if something breaks with the export hook, we'll never know -- but if we can fix nspr, it's fine with me.

Comment 15

[(not currently active) Ted Mielczarek]

Yes, I would take an NSPR patch. I don't see why the export method would be any more bound to fail than the find command.

Comment 16

[Justin Lebar (not reading bugmail)]

> I don't see why the export method would be any more bound to fail than the find 
> command.

I guess it would only be a problem if we added another custom build system and hooked it in to PGO.  Not a big deal.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 535352 [details] [diff] [review]
Tentative patch

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

This looks fine to me. I know testing this on Try will be pretty much useless, so how about we just land this on b-s and see how it goes?

If you'd like to write essentially the same patch for NSPR we can get that landed in CVS too.

::: config/rules.mk
@@ +906,5 @@
> +ifdef MOZ_PROFILE_GENERATE
> +# Clean up profiling data during PROFILE_GENERATE phase
> +export::
> +ifeq ($(OS_ARCH)_$(GNU_CC), WINNT_)
> +	-$(RM) $(wildcard *.pgd) \

$(RM) is "rm -f", so you can drop the $(wildcard) here and below.

@@ +908,5 @@
> +export::
> +ifeq ($(OS_ARCH)_$(GNU_CC), WINNT_)
> +	-$(RM) $(wildcard *.pgd) \
> +	$(if $(PROGRAM),$(DIST)/$(MOZ_APP_NAME)/$(PROGRAM:$(BIN_SUFFIX)=)!*.pgc) \
> +	$(if $(SHARED_LIBRARY),$(DIST)/$(MOZ_APP_NAME)/$(SHARED_LIBRARY_NAME)!*.pgc)

This shouldn't be necessary, pgo-merge.py removes pgc files after merging them.

Comment 18

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/projects/build-system/rev/2d1746337846

Comment 19

[Justin Lebar (not reading bugmail)]

Merged into m-c.

http://hg.mozilla.org/mozilla-central/rev/27f51c5d5f9b

Comment 20

[(not currently active) Ted Mielczarek]

I think this is probably to blame for bug 669031. We should back out this patch, or at least the Windows portion.