418772 - PGO scripts and input

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Robert Sayre]

Need to land scripts to run for PGO input. This is a tiny python server and some static data that we'll add in steps, so we can see any improvements or regressions from profiling.

Comment 1

[Robert Sayre]

Attachment: pgo script

run profileserver.py to heat up our code.

Moves a bunch of mochitest python stuff into a common area, so we can share.

Comment 2

[Robert Sayre]

Attachment: works on linux

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 304681 [details] [diff] [review]
works on linux

>Index: testing/mochitest/Makefile.in
>===================================================================
> runtests.pl: runtests.pl.in
> 	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py \
> 	$(TEST_DRIVER_PPARGS) $(DEFINES) $(ACDEFINES) $^ > $@
> 
> runtests.py: runtests.py.in
> 	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py \
> 	$(TEST_DRIVER_PPARGS) $(DEFINES) $(ACDEFINES) $^ > $@
> 
>-GARBAGE += runtests.pl runtests.py
>+automation.py: $(MOZILLA_DIR)/build/pgo/automation.py.in
>+	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py \
>+	$(TEST_DRIVER_PPARGS) $(DEFINES) $(ACDEFINES) $^ > $@
>+

Can you just s/MOZILLA_DIR/topsrcdir/ while you're here?

>Index: testing/mochitest/runtests.py.in
>===================================================================

Did your changes remove all of the preprocessor stuff?  Could this become a normal .py file instead of a .py.in now?  (I skimmed most of this file, since it was mostly code removal, might want to have Waldo take a second look at it.)


>Index: build/pgo/Makefile.in
>===================================================================
>+# The Original Code is Mozilla Communicator client code, released
>+# March 31, 1998.

Fix the ancient copyright headers here.

>+# These go in _tests/ or _profiling/ so they need to go up an extra path segment
>+AUTOMATION_PPARGS = 	\
>+			-DBROWSER_PATH=$(browser_path) \
>+			-DXPC_BIN_PATH=\"$(DIST)/bin\" \
>+			$(NULL)

I don't understand the comment here, what is it referring to?

>+automation.py: automation.py.in
>+	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py \
>+	$(AUTOMATION_PPARGS) $(DEFINES) $(ACDEFINES) $^ > $@
>+
>+profileserver.py: profileserver.py.in
>+	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py $^ > $@
>+	chmod +x $@

Maybe in Mozilla2 we can figure out how to do this stuff without having to preprocess all our Python scripts.

>Index: build/pgo/automation.py.in
>===================================================================
>+# Portions created by the Initial Developer are Copyright (C) 1998
>+# the Initial Developer. All Rights Reserved.

Just fix the year here.


>+ # These are generated in mozilla/build/Makefile.in
>+#expand DIST_BIN = "./" + __XPC_BIN_PATH__
>+#expand IS_WIN32 = len("__WIN32__") != 0
>+#expand IS_MAC = __IS_MAC__ != 0
>+#ifdef IS_CYGWIN
>+#expand IS_CYGWIN = __IS_CYGWIN__ == 1
>+#else
>+IS_CYGWIN = False
>+#endif
>+
>+UNIXISH = not IS_WIN32 and not IS_MAC

Could you file a followup on just making this use sys.platform or something instead of the preprocessor for platform detection?  Aside from the dist/bin path and the browser binary path, we should be able to detect this stuff at runtime.

>Index: build/pgo/profileserver.py.in
>===================================================================

Is there any reason this needs to be a .py.in?  I didn't see any substitutions happening.  If it wants to run from the objdir, you could just nsinstall it there.  Also, can you add a little comment at the top explaining what it does?

r=me with those changes

Comment 4

[Robert Sayre]

> 
> Can you just s/MOZILLA_DIR/topsrcdir/ while you're here?
> 
> >Index: testing/mochitest/runtests.py.in
> >===================================================================
> 
> Did your changes remove all of the preprocessor stuff?  Could this become a
> normal .py file instead of a .py.in now? 

No, because relative imports don't work when one file has been preprocessed but the other hasn't. Irritating, so I will file a follow up for that. Same for other instances of this pattern you noticed.

> I don't understand the comment here, what is it referring to?

It is stray. will fix.

> >Index: build/pgo/automation.py.in
> >===================================================================
> >+# Portions created by the Initial Developer are Copyright (C) 1998
> >+# the Initial Developer. All Rights Reserved.
> 
> Just fix the year here.
> 
> 
> >+ # These are generated in mozilla/build/Makefile.in
> >+#expand DIST_BIN = "./" + __XPC_BIN_PATH__
> >+#expand IS_WIN32 = len("__WIN32__") != 0
> >+#expand IS_MAC = __IS_MAC__ != 0
> >+#ifdef IS_CYGWIN
> >+#expand IS_CYGWIN = __IS_CYGWIN__ == 1
> >+#else
> >+IS_CYGWIN = False
> >+#endif
> >+
> >+UNIXISH = not IS_WIN32 and not IS_MAC
> 
> Could you file a followup on just making this use sys.platform or something
> instead of the preprocessor for platform detection?  Aside from the dist/bin
> path and the browser binary path, we should be able to detect this stuff at
> runtime.

Perl has problems detecting it's on Windows from msys. I don't know if Python is the same.

Comment 5

[(not currently active) Ted Mielczarek]

MozillaBuild's python is a real Windows python, not from msys, so it works fine.  I don't really care about supporting cygwin anymore anyway, but cygwin Python identifies itself as cygwin.

Comment 6

[Jeff Walden [:Waldo]]

not so much a fan of |Foo(argBegin + \n argEnd)| in MochitestServer.start -- mind changing that to either put the argument all on one line, or if you're hitting eighty characters, assign to a local first?

"Adds MochiKit chrome tests to the profile" should include a trailing period.

In bug 417075 I need to change |new RegExp('http://(.*?(:\\\\\\\\d+)?)/')| to accommodate userPass in proxied URLs.  Mind changing that to the following while you're here so I can just revert runtests.py.in locally, if you don't mind mixing up the reason for the change a touch?

  new RegExp('http://(?:[^/@]*@)?(.*?(:\\\\\\\\d+)?)/')

I kinda want the whitespace above "# write userChrome.css" to stay; the extra whitespace serves to delineate separate sections for the separate files being written out by the method.

Unindent "# These are generated in mozilla/build/Makefile.in" if that doesn't break the preprocessor (I think it doesn't).  Same for the profile setup #-delineated block.

Use all-caps for the "Run The App" block.

What's the purpose of the profileserver part?  You can't substitute it for httpd.js because Mochitests rely on httpd.js-specific behaviors like handling ^headers^ and .sjs files.  Have you actually run the entire set of Mochitests with this change?

Comment 7

[Robert Sayre]

(In reply to comment #6)
> not so much a fan of |Foo(argBegin + \n argEnd)| in MochitestServer.start --
> mind changing that to either put the argument all on one line, or if you're
> hitting eighty characters, assign to a local first?

Fixed.

> "Adds MochiKit chrome tests to the profile" should include a trailing period.
> 

Fixed.

> In bug 417075 I need to change |new RegExp('http://(.*?(:\\\\\\\\d+)?)/')| to
> accommodate userPass in proxied URLs.  Mind changing that to the following
> while you're here so I can just revert runtests.py.in locally, if you don't
> mind mixing up the reason for the change a touch?

Unrelated, fix in bug 417075.

> I kinda want the whitespace above "# write userChrome.css" to stay; the extra
> whitespace serves to delineate separate sections for the separate files being
> written out by the method.
> 
> Unindent "# These are generated in mozilla/build/Makefile.in" if that doesn't
> break the preprocessor (I think it doesn't).  Same for the profile setup
> #-delineated block.

Fixed.

> Use all-caps for the "Run The App" block.

Fixed.

> What's the purpose of the profileserver part?  You can't substitute it for
> httpd.js...

The patch does not do that. profileserver is used for PGO builds, not unit tests.

Comment 8

[Robert Sayre]

Attachment: patch for checkin

Comment 9

[Robert Sayre]

Filed bug 418896 and bug 418896 for follow-ups.

Comment 10

[Robert Sayre]

(In reply to comment #9)
> Filed bug 418896 and bug 418896 for follow-ups.
> 

also bug 418894. :)