regrtest.py improvement for Profile Guided Optimization builds

[alecsandrupatrascu]

BPO	25188
Nosy	@brettcannon
Files	regrtest_27_v01.patch regrtest_36_v01.patch regrtest_36_v02.patch issue25188.diff issue25188-py36.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/brettcannon'
closed_at = <Date 2015-10-16.19:31:36.251>
created_at = <Date 2015-09-20.05:26:22.618>
labels = ['library', 'performance']
title = 'regrtest.py improvement for Profile Guided Optimization builds'
updated_at = <Date 2015-10-16.19:31:36.249>
user = 'https://bugs.python.org/alecsandrupatrascu'

bugs.python.org fields:

activity = <Date 2015-10-16.19:31:36.249>
actor = 'brett.cannon'
assignee = 'brett.cannon'
closed = True
closed_date = <Date 2015-10-16.19:31:36.251>
closer = 'brett.cannon'
components = ['Library (Lib)']
creation = <Date 2015-09-20.05:26:22.618>
creator = 'alecsandru.patrascu'
dependencies = []
files = ['40532', '40533', '40547', '40575', '40662']
hgrepos = []
issue_num = 25188
keywords = ['patch']
message_count = 18.0
messages = ['251140', '251215', '251243', '251249', '251273', '251307', '251599', '251611', '251641', '251651', '252167', '252176', '252177', '252178', '252201', '252803', '253092', '253093']
nosy_count = 4.0
nosy_names = ['brett.cannon', 'Arfrever', 'python-dev', 'alecsandru.patrascu']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue25188'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[alecsandrupatrascu]

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.

[alecsandrupatrascu]

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.

[brettcannon]

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.

[alecsandrupatrascu]

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?

[brettcannon]

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.

[alecsandrupatrascu]

Thank you for the link. I modified regrtest.py for Python3 according to the specifications.

[brettcannon]

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).

[brettcannon]

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.

[alecsandrupatrascu]

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.

[brettcannon]

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.

[brettcannon]

Here is a patch for Python 3.6. I'll commit this after I eat before Victor changes something else. =)

[python-dev]

New changeset fb90425017e3 by Brett Cannon in branch '3.5':
Issue bpo-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 bpo-25188.
https://hg.python.org/cpython/rev/c1ecb258003b

[python-dev]

New changeset 136ad559fa4f by Brett Cannon in branch '2.7':
Issue bpo-25188: Add -P/--pgo to test.regrtest for PGO building.
https://hg.python.org/cpython/rev/136ad559fa4f

[brettcannon]

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. =)

[alecsandrupatrascu]

Thank you Brett for your effort to merge all the changes made to regrtest despite the changes appeared on the way!

[Arfrever]

New changeset 136ad559fa4f by Brett Cannon in branch '2.7':
Issue bpo-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,

[python-dev]

New changeset cc42700abb8e by Brett Cannon in branch '2.7':
Issue bpo-25188: Clean up code to pass the --pgo flag to subprocesses
https://hg.python.org/cpython/rev/cc42700abb8e

[brettcannon]

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.