classification
Title: regrtest.py improvement for Profile Guided Optimization builds
Type: performance Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Arfrever, alecsandru.patrascu, brett.cannon, python-dev
Priority: normal Keywords: patch

Created on 2015-09-20 05:26 by alecsandru.patrascu, last changed 2015-10-16 19:31 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
regrtest_27_v01.patch alecsandru.patrascu, 2015-09-21 09:20 review
regrtest_36_v01.patch alecsandru.patrascu, 2015-09-21 09:20 review
regrtest_36_v02.patch alecsandru.patrascu, 2015-09-22 11:10 review
issue25188.diff brett.cannon, 2015-09-25 19:43 review
issue25188-py36.diff brett.cannon, 2015-10-02 22:00 review
Messages (18)
msg251140 - (view) Author: Alecsandru Patrascu (alecsandru.patrascu) * Date: 2015-09-20 05:26
This issue adds improved support for Profile Guided Optimization builds in the regression test suite. 

Mainly, we keep the visual progress status in this process and suppress errors or any other unnecessary output that can alarm users.
msg251215 - (view) Author: Alecsandru Patrascu (alecsandru.patrascu) * Date: 2015-09-21 09:23
I've added the patches for Python default and 2.7. To apply them please use the command "hg import --no-commit". PGO flag is implemented for both variants (single and multi process). Default in profile-opt will be single process.
msg251243 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-09-21 17:29
I left some comments on the Python 3.6 version of the patch. I don't know if we should apply such a change to Python 2.7 or 3.5, though, as it is a new feature of regrtest and thus runs the risk of breaking stuff. Then again, we have said that anything in the test package has not backwards-compatibility guarantees.
msg251249 - (view) Author: Alecsandru Patrascu (alecsandru.patrascu) * Date: 2015-09-21 19:30
The patch does not display dots, as they are not that intuitive and users cannot make an estimate of the time left or the position in the training phase. Instead, the regrtest.py, will display the progress as [X/Y], like the default version, but without any warnings, failed tests, skipped tests. Also the start and end report are suppressed.

I don't understand what do you mean by "keyword-only parameter". I added a new flag (-P/--pgo) and propagated it along all the functions that need it. Another approach would have been to use a global variable, lets say "pgo" and use it directly. This is closer to what you had in mind?
msg251273 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-09-21 22:33
Ah, OK. As I said I had not run it so I wasn't sure of the actual outcome. =)

As for keyword-only arguments/parameters, see https://www.python.org/dev/peps/pep-3102/ . They are a Python 3 feature.
msg251307 - (view) Author: Alecsandru Patrascu (alecsandru.patrascu) * Date: 2015-09-22 11:11
Thank you for the link. I modified regrtest.py for Python3 according to the specifications.
msg251599 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-09-25 19:43
Here is a tweaked 3.6 patch so that --pgo doesn't even emit the failure count so you really can't tell what is going on unless some error info from the test spews out on to the shell (which I can't seem to stop since 2>/dev/null causes a test failure).
msg251611 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-09-25 20:55
I should mention I would have committed my patch but test_einter is hanging for me, so if someone else wants to test the patch and verify it works then I can commit it.
msg251641 - (view) Author: Alecsandru Patrascu (alecsandru.patrascu) * Date: 2015-09-26 08:05
Thank you for the tweaks. I tested the patch and it is all working fine. Without failure count the PGO run is even more transparent to the user. 

I tested your final version of the patch on several Linux server and regular desktop distributions and software configurations and I saw that test_eintr does not block and the entire PGO build finishes in good conditions every time.
msg251651 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-09-26 15:16
I fixed my test_eintr problem, but now https://hg.python.org/cpython/rev/4a9418ed0d0c landed in 3.6/default and broke the patch. Since missing getting this in before the aforementioned change was my fault, I'll personally handle making the patch apply cleanly in default.
msg252167 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-10-02 22:00
Here is a patch for Python 3.6. I'll commit this after I eat before Victor changes something else. =)
msg252176 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-10-02 23:20
New changeset fb90425017e3 by Brett Cannon in branch '3.5':
Issue #25188: Add a -P/--pgo flag to regrtest to silence error output.
https://hg.python.org/cpython/rev/fb90425017e3

New changeset c1ecb258003b by Brett Cannon in branch 'default':
Merge from 3.5 for issue #25188.
https://hg.python.org/cpython/rev/c1ecb258003b
msg252177 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-10-02 23:22
New changeset 136ad559fa4f by Brett Cannon in branch '2.7':
Issue #25188: Add -P/--pgo to test.regrtest for PGO building.
https://hg.python.org/cpython/rev/136ad559fa4f
msg252178 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-10-02 23:23
Thanks for the initial patch, Alecsandru! May I not have to touch regrtest until after Victor is done doing whatever he is doing to it in 3.6. =)
msg252201 - (view) Author: Alecsandru Patrascu (alecsandru.patrascu) * Date: 2015-10-03 08:26
Thank you Brett for your effort to merge all the changes made to regrtest despite the changes appeared on the way!
msg252803 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * (Python triager) Date: 2015-10-11 12:42
> New changeset 136ad559fa4f by Brett Cannon in branch '2.7':
> Issue #25188: Add -P/--pgo to test.regrtest for PGO building.
> https://hg.python.org/cpython/rev/136ad559fa4f
> 
> +                    # required to permit running tests with PGO flag on/off
> +                    if pgo:
> +                        args_tuple[1]['pgo']=pgo

This looks ugly.
The following change should work and would be consistent with handling of huntrleaks and use_resources:

--- Lib/test/regrtest.py
+++ Lib/test/regrtest.py
@@ -508,7 +508,8 @@
             for test in tests:
                 args_tuple = (
                     (test, verbose, quiet),
-                    dict(huntrleaks=huntrleaks, use_resources=use_resources)
+                    dict(huntrleaks=huntrleaks, use_resources=use_resources,
+                         pgo=pgo)
                 )
                 yield (test, args_tuple)
         pending = tests_and_args()
@@ -526,9 +527,6 @@
                     except StopIteration:
                         output.put((None, None, None, None))
                         return
-                    # required to permit running tests with PGO flag on/off
-                    if pgo:
-                        args_tuple[1]['pgo']=pgo
                     # -E is needed by some tests, e.g. test_import
                     popen = Popen(base_cmd + ['--slaveargs', json.dumps(args_tuple)],
                                    stdout=PIPE, stderr=PIPE,
msg253092 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-10-16 19:30
New changeset cc42700abb8e by Brett Cannon in branch '2.7':
Issue #25188: Clean up code to pass the --pgo flag to subprocesses
https://hg.python.org/cpython/rev/cc42700abb8e
msg253093 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-10-16 19:31
Thanks for the patch, Arfrever. Although next time you don't need to unset every field of the issue since the issue was still fixed, just in a suboptimal fashion that didn't impact performance.
History
Date User Action Args
2015-10-16 19:31:36brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg253093

stage: resolved
2015-10-16 19:30:28python-devsetmessages: + msg253092
2015-10-11 12:42:11Arfreversetstatus: closed -> open

nosy: + Arfrever
messages: + msg252803

resolution: fixed -> (no value)
stage: resolved -> (no value)
2015-10-03 08:26:55alecsandru.patrascusetmessages: + msg252201
2015-10-02 23:23:41brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg252178

stage: resolved
2015-10-02 23:22:44python-devsetmessages: + msg252177
2015-10-02 23:20:58python-devsetnosy: + python-dev
messages: + msg252176
2015-10-02 22:00:23brett.cannonsetfiles: + issue25188-py36.diff

messages: + msg252167
2015-09-26 15:16:43brett.cannonsetmessages: + msg251651
2015-09-26 08:05:14alecsandru.patrascusetmessages: + msg251641
2015-09-25 20:55:53brett.cannonsetassignee: brett.cannon
messages: + msg251611
2015-09-25 19:43:29brett.cannonsetfiles: + issue25188.diff

messages: + msg251599
2015-09-22 11:11:39alecsandru.patrascusetmessages: + msg251307
2015-09-22 11:10:34alecsandru.patrascusetfiles: + regrtest_36_v02.patch
2015-09-21 22:33:29brett.cannonsetmessages: + msg251273
2015-09-21 19:30:54alecsandru.patrascusetmessages: + msg251249
2015-09-21 17:29:13brett.cannonsetmessages: + msg251243
2015-09-21 09:23:40alecsandru.patrascusetmessages: + msg251215
2015-09-21 09:20:34alecsandru.patrascusetfiles: + regrtest_36_v01.patch
2015-09-21 09:20:26alecsandru.patrascusetfiles: + regrtest_27_v01.patch
keywords: + patch
2015-09-20 18:29:13brett.cannonlinkissue24915 dependencies
2015-09-20 18:28:13brett.cannonsetnosy: + brett.cannon
2015-09-20 05:26:22alecsandru.patrascucreate