Closed
Bug 659942
Opened 14 years ago
Closed 13 years ago
Clean up profiling data during PROFILE_GENERATE phase
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: glandium, Assigned: glandium)
References
Details
(Whiteboard: [fixed-in-build-system])
Attachments
(2 files, 1 obsolete file)
1.89 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
ted
:
review-
|
Details | Diff | Splinter Review |
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•14 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!)
Assignee | ||
Comment 3•14 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.
Comment 4•14 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.
Comment 5•14 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.
Comment 6•14 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
Comment 7•14 years ago
|
||
Updated•14 years ago
|
Attachment #538387 -
Flags: review?(ted.mielczarek)
Comment 8•14 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.
Comment 9•14 years ago
|
||
Updated•14 years ago
|
Attachment #538387 -
Attachment is obsolete: true
Attachment #538387 -
Flags: review?(ted.mielczarek)
Comment 10•14 years ago
|
||
Comment on attachment 538407 [details] [diff] [review]
Patch v2
Okay, how about this?
Attachment #538407 -
Attachment description: Clean up old profiling data before running the instrumented build during PGO. → Patch v2
Attachment #538407 -
Flags: review?(ted.mielczarek)
Comment 11•14 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.
Comment 12•14 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.
Comment 13•14 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.
Comment 14•14 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.
Comment 15•14 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.
Comment 16•14 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.
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.
Attachment #535352 -
Flags: review+
Updated•13 years ago
|
Attachment #538407 -
Flags: review?(ted.mielczarek) → review-
Comment 18•13 years ago
|
||
Whiteboard: [fixed-in-build-system]
Comment 19•13 years ago
|
||
Merged into m-c.
http://hg.mozilla.org/mozilla-central/rev/27f51c5d5f9b
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Blocks: 669031
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•