Mike Hommey [:glandium] Assignee
	Description • 13 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 1 • 13 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 2 • 13 years ago

This needs testing.

	Mike Hommey [:glandium] Assignee
	Comment 3 • 13 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 4 • 13 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 5 • 13 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 6 • 13 years ago

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

	(not currently active) Ted Mielczarek
	Comment 8 • 13 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 10 • 13 years ago

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

Okay, how about this?

	Justin Lebar (not reading bugmail)
	Comment 11 • 13 years ago

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.

	(not currently active) Ted Mielczarek
	Comment 12 • 13 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 13 • 13 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 14 • 13 years ago

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.

	(not currently active) Ted Mielczarek
	Comment 15 • 13 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 16 • 13 years ago

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

	(not currently active) Ted Mielczarek
	Comment 17 • 13 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 18 • 13 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 19 • 13 years ago

Merged into m-c.

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

	(not currently active) Ted Mielczarek
	Comment 20 • 13 years ago

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