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.

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!)

This needs testing.

(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.

(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.

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.

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

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 on attachment 538407 [details] [diff] [review] Patch v2 Okay, how about this?

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.

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.

> 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.

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.

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.

> 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 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.

Merged into m-c. http://hg.mozilla.org/mozilla-central/rev/27f51c5d5f9b

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