Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regrtest.py improvement for Profile Guided Optimization builds #69375

Closed
alecsandrupatrascu mannequin opened this issue Sep 20, 2015 · 18 comments
Closed

regrtest.py improvement for Profile Guided Optimization builds #69375

alecsandrupatrascu mannequin opened this issue Sep 20, 2015 · 18 comments
Assignees
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@alecsandrupatrascu
Copy link
Mannequin

alecsandrupatrascu mannequin commented Sep 20, 2015

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
    Copy link
    Mannequin Author

    alecsandrupatrascu mannequin commented Sep 20, 2015

    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 alecsandrupatrascu mannequin added stdlib Python modules in the Lib dir performance Performance or resource usage labels Sep 20, 2015
    @alecsandrupatrascu
    Copy link
    Mannequin Author

    alecsandrupatrascu mannequin commented Sep 21, 2015

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    alecsandrupatrascu mannequin commented Sep 21, 2015

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    alecsandrupatrascu mannequin commented Sep 22, 2015

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

    @brettcannon
    Copy link
    Member

    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
    Copy link
    Member

    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.

    @brettcannon brettcannon self-assigned this Sep 25, 2015
    @alecsandrupatrascu
    Copy link
    Mannequin Author

    alecsandrupatrascu mannequin commented Sep 26, 2015

    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
    Copy link
    Member

    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
    Copy link
    Member

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 2, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Oct 2, 2015

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    alecsandrupatrascu mannequin commented Oct 3, 2015

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

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Oct 11, 2015

    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,

    @Arfrever Arfrever mannequin reopened this Oct 11, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 16, 2015

    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
    Copy link
    Member

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant