Closed Bug 468285 Opened 16 years ago Closed 6 years ago

Components cannot be PGO'd on Windows

Categories

(Firefox Build System :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: chris.bugzilla, Assigned: chris.bugzilla)

Details

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

The .pgc files that hold the optimziation information get deleted before being used to optimize the application.

The problem is that the components dlls and their .pgc files are in the objdir/dist/bin/components folder and this folder gets clobber at the beginning of each build.

A profiledbuild is 2 builds - the first build has an environment variable of MOZ_PROFILE_GENERATE=1 and the second build has an environment variable of MOZ_PROFILE_USE=1.

so the objdir/dist/bin/components folder gets clobbered at the beginning of each of the builds.  With the profile data being in this folder it gets deleted before it is used.

Reproducible: Always

Steps to Reproduce:
1. 
  make -f client.mk build MOZ_PROFILE_GENERATE=1
2.
  objdir/dist/bin/firefox
  close firefox
3.
  look at the files in the objdir/dist/bin/components
  there will be many *.pgc - on windows leave open windows explorer at this folder.
4.
  make -f client.mk build MOZ_PROFILE_USE=1

You'll notice that right away this folder gets cleared out.  If you have the build output going to your screen you'll notice that a rm -rf dist/bin/components is displayed near the top.
Ted on irc mentioned that in the topsrcdir/Makefile.in file (near line 52) the rm -rf dist/bin/components could be wrapped in a ifndef MOZ_PROFILE_USE then it would not delete the files.  This is safe since first build would clobber that directory clearing out any old components.

This won't solve the problem since in config/rules.mk file (near 845 or 947) the merging of pgc files into their corresponding pgd files only happens in the dist/bin folder.

I suggest duplicating the code to look in the dist/bin/components since the pgomerge.py file does not return errors for files not found.

export::
	$(PYTHON) $(topsrcdir)/build/win32/pgomerge.py \
	  $(BINARY_BASENAME) $(DIST)/bin
  $(PYTHON) $(topsrcdir)/build/win32/pgomerge.py \
	  $(BINARY_BASENAME) $(DIST)/bin/components

I'm testing this out to see if it works.
I'd rather not run the script twice. I'd accept a patch that either passed both dist/bin and dist/bin/components to pgomerge.py on the commandline, and modified the script to acommodate, or a patch that made pgomerge.py look for the matching pgc file recursively under the directory passed in on the commandline.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Alright, I will have to learn python.  I'm currently testing to see if I run the script twice if it will do the trick.  I just want to confirm that pointing it to the components directory will solve the problem before modifying the pgomerge.py script.
This is my first attempt to check all sub directories for pgc files. At the time of writing this I have not tested it on a pgo build only in small test scripts.

This script will loop through all subdirectories...I need a flag that will return after successfully finding any files in one directory.  I'll add that in my next iteration.
Attached file Corrected mime-type (obsolete) —
I left the content type as auto-detect instead of specifying plain text.  This file can be viewed without downloading.
Attachment #351838 - Attachment is obsolete: true
Note that if you click the "details" link next to a patch, you can change the mime type. :)

As for your actual patch, looks pretty reasonable, although I just gave it a quick glance. You could factor that loop code out into a separate function, something like:
def FindPGCFile(name, dir):
  ...

and have that function return the filename when it finds it, which would get you out of the loop early, or return None at the end of the function. Then the code calling the function could just ensure that it gets something that's not None, and run pgomerge on it.
thanks ted...I have some logic errors in it still. Hopefully I have it up and running today.
Attached file updated pgomerge.py (obsolete) —
This uses a function to get a list of pgc files that it found.
Attachment #351839 - Attachment is obsolete: true
Attached patch Correct pgomerge.py diff file (obsolete) — Splinter Review
Sorry...I was moving too fast.  I grabbed an old file in a different directory.
Attachment #351961 - Attachment is obsolete: true
Attachment #351963 - Attachment is patch: true
Attachment #351963 - Flags: review?(ted.mielczarek)
Assignee: nobody → chris.bugzilla
Looking in the sub folders for pgc files is part 2 of this problem.  Part 1 is that these files get deleted.  I have a patch to fix that. Should that be a new bug or part of this bug?
Just put it on this bug, that's fine.
Attached patch stop deleting profile data (obsolete) — Splinter Review
This is the patch for the other part of this problem.  The profile data being deleted.

This checks if the current build operation is not MOZ_PROFILE_USE,if not then go ahead and delete the dist/bin/components folder.

If this is the MOZ_PROFILE_USE then don't delete the folder...it has already been cleaned out and it holds any components that have been profiled.
Attachment #352144 - Flags: review?(ted.mielczarek)
Attachment #351963 - Attachment is obsolete: true
Attachment #352144 - Attachment is obsolete: true
Attachment #352150 - Flags: review?(ted.mielczarek)
Attachment #351963 - Flags: review?(ted.mielczarek)
Attachment #352144 - Flags: review?(ted.mielczarek)
The pgomerge.py looks like I made unnecessary to some lines but I kept getting indent errors in python.  My editor allowed me to view tabs and whitespace and I noticed alot of whitespace.  So I changed the whitespaces to tabs. This fixed my problem.

This might not have been the correct approach.
Comment on attachment 352150 [details] [diff] [review]
patch to stop deleting profile data and look for pgc files in subdirs

+ifndef MOZ_PROFILE_USE
 	$(RM) -rf $(DIST)/bin/components
+endif
+
 	$(RM) -rf _tests

Please get rid of that extra blank line.

pgomerge.py doesn't use tabs at all, it uses spaces with two-space indentation. Please replace all tabs with two spaces and make sure the indentation is lined up properly.

+def GetPGCList(basename, startDir):

Can you name this FindPGCFiles, to mirror the MergePGOFiles below?

+		if len(found) > 0:
+			return found

You can write this as:
if found:
  return found

+	for pgcfile in files:
+		try:
+			subprocess.call(['pgomgr', '-merge',
+												pgcfile,
+												pgdfile])
+			os.remove(pgcfile)
+		except OSError:
+			pass

You can do this without a loop, since pgomgr will take a list of pgc files on the commandline. ( http://msdn.microsoft.com/en-us/library/2kw46d8w%28VS.80%29.aspx ).

I'd do something like:
files = GetPGCList(basename, pgcdir)
if files:
  command = ['pgomgr', '-merge']
  command.extend(files)
  command.append(pgdfile)
  subprocess.call(command)

Of course with the same try/except block wrapped around the call part.

r-, but only really to fix up the mess with tabs. Otherwise it looks pretty good. Get a patch up with those changes and you'll get an r+.
Attachment #352150 - Flags: review?(ted.mielczarek) → review-
triaging, closing inactive bug
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: