Skip to content
This repository was archived by the owner on Sep 10, 2022. It is now read-only.

Conversation

@asottile
Copy link
Member

@asottile asottile commented Nov 7, 2017

This adds about 14 minutes to the build on my machine (which seems reasonable?)

(these numbers are against ubuntu/xenial branch)
(before): real 12m30.493s
(after): real 26m46.294s

Not all of the test modules pass during profiling for metest_shutil test___all__

The test_shutil failure appears to be due to docker:

ERROR: test_copyxattr_symlinks (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.6/test/test_shutil.py", line 489, in test_copyxattr_symlinks
    os.setxattr(src, 'trusted.foo', b'42')
PermissionError: [Errno 1] Operation not permitted: '/tmp/tmpq5vt2tn8/foo'

----------------------------------------------------------------------

test___all__ fails due to this:

0:00:00 load avg: 0.11 [1/1] test___all__
Warning -- warnings.filters was modified by test___all__
  Before: (140609970216072, [('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'BytesWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0)], [('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'BytesWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0)])
  After:  (140609970216072, [('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'BytesWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0)], [('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'BytesWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0), ('ignore', re.compile('', re.IGNORECASE), <class 'pkg_resources.PEP440Warning'>, re.compile(''), 0)]) 
test___all__ failed (env changed)

1 test altered the execution environment:
    test___all__

But this is ok, because of this line

@asottile
Copy link
Member Author

asottile commented Nov 7, 2017

CC @rowillia @chriskuehl @fkrull

@fkrull
Copy link
Member

fkrull commented Dec 23, 2017

Well, for some reason, the build takes several hours for me with this change, most of that running the tests. I'm not sure why that is; this isn't exactly a slow laptop, but looking at the VM config, it only has 4GB of memory, so I might have forced it so swap. Either way, your numbers are much more reasonable.

In any case, the automatic package tests pass, so all the same tests as before seem to be succeeding/failing. I say it's safe to turn on. Probably.

@fkrull fkrull merged commit 9ccd0e5 into master Dec 23, 2017
@asottile asottile deleted the pgo branch December 23, 2017 18:05
@asottile
Copy link
Member Author

I'm going to run this locally for a while and then cut a release. I might cut out some of the slower tests too. It's taking much longer on my laptop to build heh

@asottile
Copy link
Member Author

asottile commented Dec 25, 2017

I looked at this some more, my timings above must have been before I added the flag to ./configure.

I also noticed that the profile test exclusions were only being applied to the python build and not for the libpython / debug / debug libpython builds.

I'll have another branch soon but tl;dr after fixing the excludes and removing profiled builds for the two debug builds it clocks in at ~28 minutes of overhead (essentially 2 test runs worth).

I'm going to do some benchmarking and see if it's worth it. I also wonder if we can reduce the four builds into two, though I'm probably overlooking some flag differences scratch that, I see what's happening after looking closer 😆

@asottile
Copy link
Member Author

Here's the results of the performance tests I ran: https://i.fluffy.cc/xDgvxN1qW554V3q92GNPl7Gh1GxpWwKX.html

Overall a 5-30% improvement on all of the benchmarks!

@asottile asottile mentioned this pull request Dec 26, 2017
@fkrull
Copy link
Member

fkrull commented Dec 27, 2017

As far as I understand, the static build is so the main interpreter doesn't depend on the shared library to keep it more self-contained. But that seems like a reason that's more relevant to Python as a part of a minimal bootstrapped OS.

Though in general, I recommend keeping the changes to the upstream package to a minimum. Any change, however small it might seem, has a tendency to multiply once you have to keep any changes in sync between different Python versions and different Ubuntu versions. My experience has been that once you start changing things ("we don't need that part", "this could be solved better", "ooooh, I want that feature"), it ends up in a sort of maintenance nightmare.

PGO is a pretty small change with an actual measurable payoff, so it should be fine. I just wanted to add my general thoughts.

@asottile
Copy link
Member Author

After trying these out for a while, I decided to upload them today.

Strangely, both amd64 and i386 builds failed for only trusty. I'm building it locally and it has gotten past the place where it failed. I wonder if it's a bad builder host or if -fprofile-generate is buggy in trusty. I'm going to try uploading a trivial version bump on top of it as a "retry", and if that fails I'll revert pgo in trusty.

@asottile
Copy link
Member Author

actually, if I log in I can click retry, trying that first :)

@asottile
Copy link
Member Author

Failed again in the same way, I guess I'll revert this for trusty.

amd64

changing mode of build/scripts-3.6/pyvenv from 664 to 775
renaming build/scripts-3.6/pydoc3 to build/scripts-3.6/pydoc3.6
renaming build/scripts-3.6/idle3 to build/scripts-3.6/idle3.6
renaming build/scripts-3.6/2to3 to build/scripts-3.6/2to3-3.6
renaming build/scripts-3.6/pyvenv to build/scripts-3.6/pyvenv-3.6
Fatal Python error: unexpected exception during garbage collection

Current thread 0x00002b49d4f63e00 (most recent call first):
Exception ignored in: 'garbage collection'
SystemError: ../Objects/unicodeobject.c:2344: bad argument to internal function
Aborted (core dumped)
make[3]: *** [sharedmods] Error 134
make[3]: Leaving directory `/<<PKGBUILDDIR>>/build-static'
make[2]: *** [build_all_generate_profile] Error 2
make[2]: Leaving directory `/<<PKGBUILDDIR>>/build-static'
make[1]: *** [profile-opt] Error 2
make[1]: Leaving directory `/<<PKGBUILDDIR>>/build-static'
make: *** [stamps/stamp-build-static] Error 2
dpkg-buildpackage: error: debian/rules build-arch gave error exit status 2
--------------------------------------------------------------------------------

i386


changing mode of build/scripts-3.6/pyvenv from 664 to 775
renaming build/scripts-3.6/pydoc3 to build/scripts-3.6/pydoc3.6
renaming build/scripts-3.6/idle3 to build/scripts-3.6/idle3.6
renaming build/scripts-3.6/2to3 to build/scripts-3.6/2to3-3.6
renaming build/scripts-3.6/pyvenv to build/scripts-3.6/pyvenv-3.6
*** Error in `./python': corrupted size vs. prev_size: 0x09226888 ***
Aborted (core dumped)
make[3]: *** [sharedmods] Error 134
make[3]: Leaving directory `/<<PKGBUILDDIR>>/build-static'
make[2]: *** [build_all_generate_profile] Error 2
make[2]: Leaving directory `/<<PKGBUILDDIR>>/build-static'
make[1]: *** [profile-opt] Error 2
make[1]: Leaving directory `/<<PKGBUILDDIR>>/build-static'
make: *** [stamps/stamp-build-static] Error 2
dpkg-buildpackage: error: debian/rules build gave error exit status 2

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants