
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2017-10-04 15:42 byishcherb, last changed2022-04-11 14:58 byadmin. This issue is nowclosed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| 00125-less-verbose-COUNT_ALLOCS.patch | ishcherb,2017-10-06 20:36 | |||
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 3910 | closed | vstinner,2017-10-06 19:58 | |
| PR 3927 | merged | vstinner,2017-10-09 09:21 | |
| PR 4019 | merged | vstinner,2017-10-17 13:55 | |
| Messages (23) | |||
|---|---|---|---|
| msg303705 -(view) | Author: Iryna Shcherbina (ishcherb)* | Date: 2017-10-04 15:42 | |
The newly added `test_huntrleaks` test is failing on Python 2.7.14 debug build with COUNT_ALLOCS.======================================================================FAIL: test_huntrleaks (test.test_regrtest.ArgsTestCase)----------------------------------------------------------------------Traceback (most recent call last): File "/builddir/build/BUILD/Python-2.7.14/Lib/test/test_regrtest.py", line 511, in test_huntrleaks self.check_leak(code, 'references') File "/builddir/build/BUILD/Python-2.7.14/Lib/test/test_regrtest.py", line 489, in check_leak self.assertIn(line2, output)AssertionError: 'test_regrtest_huntrleaks leaked [1, 1, 1] references, sum=3\n' not found in 'Run tests sequentially\n0:00:00 load avg: 0.63 [1/1] test_regrtest_huntrleaks\nbeginning 6 repetitions\n123456\n......\ntest_regrtest_huntrleaks leaked [93, 93, 93] references, sum=279\n1 test failed:\n test_regrtest_huntrleaks\n\nTotal duration: 32 ms\nTests result: FAILURE\n[53092 refs]\n'----------------------------------------------------------------------On Python 2.7.14 debug build *without* COUNT_ALLOCS the test passes fine, therefore I am not sure if there is a leak detected, or is it just COUNT_ALLOCS messing with the test and it should be skipped. | |||
| msg303821 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-06 14:20 | |
test_regrtest is not the single test failing when COUNT_ALLOCS is defined. Not least than 12 tests are failing.In Python 3, many tests were fixed by skipping them if COUNT_ALLOCS is defined: commita793037d803abf098d172f686e2b95d27863c54d ofbpo-19527.Since Python 3.6, even if COUNT_ALLOCS is defined, statistics are no more dumped by default, but only if -X showalloccount is defined: commit7e160ce356036bccda2608d9ee10bfe276dfa97a ofbpo-23034. Moreover, statitics are now written into stderr rather than stdout. | |||
| msg303828 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2017-10-06 15:28 | |
Commita793037d803abf098d172f686e2b95d27863c54d is not related to this issue. It skips the tests that were failed due to other side effect of COUNT_ALLOCS -- it makes type immortal. This is not a problem in 2.7 since all types are immortal in 2.7.But there is other patch inissue19527: 00141-fix-tests_with_COUNT_ALLOCS-v5.patch. It was not committed sinceissue23034 provided better solution. You can try to backport it to 2.7, but this will just complicate the testing code for almost no benefit. Nobody wanted to do this. It was a decision to ignore this issue in Python 3.5 and earlier. | |||
| msg303848 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-06 20:02 | |
I wrotePR 3910 to modify COUNT_ALLOCS: alllcations statistics are now written into stderr, rather than stdout.The PR contains also changes to fix tests with COUNT_ALLOCS. I'm not sure about this part of the PR. I would prefer to not dump statistics by default, and add a new option to enable it.I don't know if we can easily implement "-X showalloccount", since Python 2.7 doesn't have sys._xoptions. Maybe we could use a new environment variable instead: PYTHONSHOWALLOCCOUNT=1?I would prefer to add a new option instead of having to patch so many unit tests:haypo@selma$ git diff 2.7.. --statLib/json/tests/test_tool.py | 8 ++++++--Lib/test/support/__init__.py | 3 +++Lib/test/test_abc.py | 2 ++Lib/test/test_gc.py | 4 +++-Lib/test/test_hash.py | 2 +-Lib/test/test_module.py | 3 ++-Lib/test/test_multiprocessing.py | 4 +++-Lib/test/test_regrtest.py | 1 +Lib/test/test_subprocess.py | 17 +++++++++++++++++Lib/test/test_sys.py | 21 +++++++++++++++------Lib/test/test_threading.py | 11 ++++++++---Lib/test/test_tools.py | 10 +++++++---Lib/test/test_warnings.py | 4 ++++Lib/test/test_weakref.py | 1 +Misc/NEWS.d/next/Core and Builtins/2017-10-06-21-56-47.bpo-31692.osJuVJ.rst | 2 ++Python/pythonrun.c | 2 +- 16 files changed, 76 insertions(+), 19 deletions(-) | |||
| msg303851 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2017-10-06 20:21 | |
Do you going to backport also -X showrefcount? | |||
| msg303852 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-06 20:24 | |
My colleague Iryna Shcherbina came to me with this issue. I'm not sure that we really need to support COUNT_ALLOCS. But strangely, it seems simpler to fix bugs rather than guessing if users like this debug mode or not :-)There are different ways to fix tests to make them passing with COUNT_ALLOCS. As I wrote, I would prefer to add a new Python 2.7 option to explicitly asks to dump allocations statistics: add a new PYTHONSHOWALLOCCOUNT=1 environment variable for example.What do you think Serhiy? Do you prefer to decorate many 2.7 tests to skip them if COUNT_ALLOCS is used, or to add a new option?I should try to implement -X showalloccount :-) | |||
| msg303855 -(view) | Author: Iryna Shcherbina (ishcherb)* | Date: 2017-10-06 20:36 | |
> I don't know if we can easily implement "-X showalloccount", since> Python 2.7 doesn't have sys._xoptions. Maybe we could use a new> environment variable instead: PYTHONSHOWALLOCCOUNT=1?That is how it is bypassed in Fedora Py2 builds currently. I am attaching the patch we use. | |||
| msg303858 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2017-10-06 20:47 | |
I left this on to Benjamin, the RM of Python 2.7. | |||
| msg303859 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-06 20:58 | |
Serhiy:> Do you going to backport also -X showrefcount?I see -X as a Python3 only thing. If I have to choose, I would prefer to add a new environment variable, since it's more in the "Python2 style".Iryna:> That is how it is bypassed in Fedora Py2 builds currently. I am attaching the patch we use.Oh, nice to see that we get the same idea: opt-in environment variable.I'm unhappy with the variable name: PYTHONDUMPCOUNTS seems too generic IMHO :-(I prefer "PYTHONSHOWALLOCCOUNT" since it's closer to Python 3 "-X showalloccount" option, and "alloc" mentions that they are counters on allocations. Not just "random" counters. | |||
| msg303933 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-09 09:22 | |
Ok, so. I wrote a different PR:PR 3927 adds a new PYTHONSHOWALLOCCOUNT environment variable and write allocations statistics into stderr (rather than stdout) when PYTHONSHOWALLOCCOUNT is set.The PR also fixes the test suite for COUNT_ALLOCS.ThePR 3927 is much shorter because PYTHONSHOWALLOCCOUNT now has to be defined to dump allocations statistics.haypo@selma$ git diff 2.7.. --statDoc/using/cmdline.rst | 7 +++++++Lib/test/support/__init__.py | 3 +++Lib/test/test_abc.py | 1 +Lib/test/test_gc.py | 4 +++-Lib/test/test_regrtest.py | 1 +Lib/test/test_sys.py | 5 ++++-Lib/test/test_weakref.py | 1 +Misc/NEWS.d/next/Core and Builtins/2017-10-09-11-03-13.bpo-31692.5-bpdk.rst | 4 ++++Python/pythonrun.c | 4 +++- 9 files changed, 27 insertions(+), 3 deletions(-) | |||
| msg303937 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-09 09:38 | |
I testedPR 3927. I run "./python -m test -j8 -r" with PYTHONSHOWALLOCCOUNT unset: the full Python test suite pass in release and debug modes:* ./configure CFLAGS="-D COUNT_ALLOCS=1"* ./configure --with-pydebug CFLAGS="-D COUNT_ALLOCS=1" | |||
| msg303944 -(view) | Author: Iryna Shcherbina (ishcherb)* | Date: 2017-10-09 12:26 | |
I have run a test build with the patch fromPR 3927 applied, and all tests passed.Full build log can be viewed here:https://kojipkgs.fedoraproject.org//work/tasks/8392/22348392/build.log | |||
| msg303946 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2017-10-09 12:33 | |
Adding PYTHONSHOWALLOCCOUNT looks rather like a new feature to me. It is on to Bejaming to make a decision about this.The difference between an environment variable and a command line option is that the former is inherited by children, and the latter is not (this can be considered a drawback or an advantage).COUNT_ALLOCS is not the only option that spoils Python output. Py_TRACE_REFS adds even more noise to stderr, and this is a default option in debug build. How many tests are still left broken if just change the COUNT_ALLOCS output to stderr instead of stdout? | |||
| msg303956 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-09 13:48 | |
Serhiy: "Do you going to backport also -X showrefcount?"I just createdbpo-31733 "Add PYTHONSHOWREFCOUNT environment variable to Python 2.7". | |||
| msg303957 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-09 13:49 | |
> How many tests are still left broken if just change the COUNT_ALLOCS output to stderr instead of stdout?That's my first PR:PR 3910.> COUNT_ALLOCS is not the only option that spoils Python output. Py_TRACE_REFS adds even more noise to stderr, and this is a default option in debug build.Again, I createdbpo-31733 to propose to make this *super annoying* [xxx refs] line *by default*. It makes this issue more consistent ;-) | |||
| msg303959 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-09 13:55 | |
(I prefer to not split the discussion on this issue and my 2 PR, I prefer to only discuss the design in this issue.)https://github.com/python/cpython/pull/3927#pullrequestreview-67963845Serhiy:> Using the requires_type_collecting decorator LGTM. But I don't sure about adding PYTHONSHOWALLOCCOUNT. This looks like a new feature to me.> What if split this PR on two parts? The one fixes issues related to immortal types, the other is about additional output.Both changes are connected. If you only want to fix tests, that's the purpose of my much longerPR 3910."This looks like a new feature to me."My PR changes the default behaviour, but COUNT_ALLOCS is not enabled by default, not even by the debug mode. You have to explicitely enable it. I expect that developers who use this option are able to track the Python 2.7 changelog and "discover" the newly added environment variable by themself. Moreover, defining PYTHONSHOWALLOCCOUNT=1 is "backward compatible" with Python < 2.7.15 and Python 3, since unknown environment variables are just ignored. | |||
| msg303961 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2017-10-09 14:14 | |
All Victor's PRs LGTM is they look good to Benjamin.But on the next iteration we can get a report that tests don't work if set PYTHONSHOWREFCOUNT. | |||
| msg304493 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-17 09:25 | |
New changeset7b4ba62e388474e811268322b47f80d464933541 by Victor Stinner in branch '2.7':[2.7]bpo-31692: Add PYTHONSHOWALLOCCOUNT env var (GH-3927)https://github.com/python/cpython/commit/7b4ba62e388474e811268322b47f80d464933541 | |||
| msg304495 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-17 10:03 | |
Python 2.7.15 will require to compile Python with COUNT_ALLOCS defined *and* to set the PYTHONSHOWALLOCCOUNT environment variable (ex: PYTHONSHOWALLOCCOUNT=1) to dump allocation counts. Moreover, counts are now dumped into stderr, instead of stdout.Python 2.7 test suite now pass when Python is compiled with COUNT_ALLOCS.Python 2.7 test suite doens't pass with PYTHONSHOWALLOCCOUNT set. I don't think that it's worth it to fix this case. | |||
| msg304497 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2017-10-17 10:17 | |
Isn't worth to document these changes in What's New? | |||
| msg304509 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-17 13:56 | |
Serhiy: "Isn't worth to document these changes in What's New?"Ok, I created thePR 4019. | |||
| msg304527 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-17 20:13 | |
New changeset355393e7438deeab4aeec3fcffea65e8cada083b by Victor Stinner in branch '2.7':[2.7]bpo-31733,bpo-31692: Document 2 new env vars in What's New in Python 2.7 (GH-4019)https://github.com/python/cpython/commit/355393e7438deeab4aeec3fcffea65e8cada083b | |||
| msg304528 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2017-10-17 20:14 | |
I documented the two new environment variables in What's New in Python 2.7, so this issue can now be closed again. | |||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:58:53 | admin | set | github: 75873 |
| 2017-10-17 20:14:08 | vstinner | set | status: open -> closed resolution: fixed messages: +msg304528 |
| 2017-10-17 20:13:15 | vstinner | set | messages: +msg304527 |
| 2017-10-17 13:56:59 | vstinner | set | status: closed -> open resolution: fixed -> (no value) messages: +msg304509 |
| 2017-10-17 13:55:57 | vstinner | set | pull_requests: +pull_request3994 |
| 2017-10-17 10:17:44 | serhiy.storchaka | set | messages: +msg304497 |
| 2017-10-17 10:03:02 | vstinner | set | status: open -> closed resolution: fixed messages: +msg304495 stage: patch review -> resolved |
| 2017-10-17 09:25:28 | vstinner | set | messages: +msg304493 |
| 2017-10-09 14:14:40 | serhiy.storchaka | set | messages: +msg303961 |
| 2017-10-09 13:55:12 | vstinner | set | messages: +msg303959 |
| 2017-10-09 13:49:59 | vstinner | set | messages: +msg303957 |
| 2017-10-09 13:48:47 | vstinner | set | messages: +msg303956 |
| 2017-10-09 12:33:04 | serhiy.storchaka | set | messages: +msg303946 |
| 2017-10-09 12:26:31 | ishcherb | set | messages: +msg303944 |
| 2017-10-09 09:38:27 | vstinner | set | messages: +msg303937 |
| 2017-10-09 09:22:53 | vstinner | set | messages: +msg303933 |
| 2017-10-09 09:21:51 | vstinner | set | pull_requests: +pull_request3901 |
| 2017-10-06 20:58:45 | vstinner | set | messages: +msg303859 |
| 2017-10-06 20:47:27 | serhiy.storchaka | set | messages: +msg303858 |
| 2017-10-06 20:36:32 | ishcherb | set | files: +00125-less-verbose-COUNT_ALLOCS.patch messages: +msg303855 |
| 2017-10-06 20:24:33 | vstinner | set | messages: +msg303852 |
| 2017-10-06 20:21:23 | serhiy.storchaka | set | messages: +msg303851 |
| 2017-10-06 20:19:52 | serhiy.storchaka | set | nosy: +benjamin.peterson |
| 2017-10-06 20:02:02 | vstinner | set | messages: +msg303848 |
| 2017-10-06 19:58:09 | vstinner | set | keywords: +patch stage: patch review pull_requests: +pull_request3882 |
| 2017-10-06 15:28:02 | serhiy.storchaka | set | nosy: +serhiy.storchaka messages: +msg303828 |
| 2017-10-06 14:20:31 | vstinner | set | nosy: +vstinner messages: +msg303821 title: Test `test_huntrleaks` fails in debug build with COUNT_ALLOCS -> [2.7] Test `test_huntrleaks()` of test_regrtest fails in debug build with COUNT_ALLOCS |
| 2017-10-05 15:43:34 | cstratak | set | nosy: +cstratak |
| 2017-10-04 15:42:48 | ishcherb | create | |