Last Comment Bug 659942 - Clean up profiling data during PROFILE_GENERATE phase
: Clean up profiling data during PROFILE_GENERATE phase
Status: RESOLVED FIXED
[fixed-in-build-system]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 665978 679334
Blocks: 659311 663251 669031
  Show dependency treegraph
 
Reported: 2011-05-26 08:12 PDT by Mike Hommey [:glandium]
Modified: 2012-01-03 03:44 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tentative patch (1.89 KB, patch)
2011-05-26 08:40 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
Like so? (2.97 KB, patch)
2011-06-09 17:08 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Patch v2 (2.50 KB, patch)
2011-06-09 18:12 PDT, Justin Lebar (not reading bugmail)
ted: review-
Details | Diff | Review

Description Mike Hommey [:glandium] 2011-05-26 08:12:24 PDT
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) 2011-05-26 08:32:22 PDT
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] 2011-05-26 08:40:42 PDT
Created attachment 535352 [details] [diff] [review]
Tentative patch

This needs testing.
Comment 3 Mike Hommey [:glandium] 2011-05-26 08:50:44 PDT
(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) 2011-06-08 21:25:13 PDT
(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) 2011-06-09 13:46:58 PDT
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) 2011-06-09 17:02:39 PDT
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) 2011-06-09 17:08:54 PDT
Created attachment 538387 [details] [diff] [review]
Like so?
Comment 8 Ted Mielczarek [:ted.mielczarek] 2011-06-09 17:19:24 PDT
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) 2011-06-09 18:12:40 PDT
Created attachment 538407 [details] [diff] [review]
Patch v2
Comment 10 Justin Lebar (not reading bugmail) 2011-06-09 18:13:35 PDT
Comment on attachment 538407 [details] [diff] [review]
Patch v2

Okay, how about this?
Comment 11 Justin Lebar (not reading bugmail) 2011-06-10 09:28:48 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-06-10 10:34:45 PDT
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) 2011-06-10 10:50:44 PDT
> 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) 2011-06-10 11:28:57 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-06-10 11:39:26 PDT
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) 2011-06-10 11:53:08 PDT
> 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 Ted Mielczarek [:ted.mielczarek] 2011-06-13 05:44:15 PDT
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) 2011-06-13 10:29:08 PDT
http://hg.mozilla.org/projects/build-system/rev/2d1746337846
Comment 19 Justin Lebar (not reading bugmail) 2011-06-14 12:36:39 PDT
Merged into m-c.

http://hg.mozilla.org/mozilla-central/rev/27f51c5d5f9b
Comment 20 Ted Mielczarek [:ted.mielczarek] 2011-08-16 06:48:18 PDT
I think this is probably to blame for bug 669031. We should back out this patch, or at least the Windows portion.

Note You need to log in before you can comment on or make changes to this bug.