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

Test failures with COUNT_ALLOCS #63726

Closed
bkabrda mannequin opened this issue Nov 8, 2013 · 19 comments
Closed

Test failures with COUNT_ALLOCS #63726

bkabrda mannequin opened this issue Nov 8, 2013 · 19 comments
Assignees
Labels
tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@bkabrda
Copy link
Mannequin

bkabrda mannequin commented Nov 8, 2013

BPO 19527
Nosy @vstinner, @serhiy-storchaka, @rkuska
PRs
  • [2.7] bpo-31692: COUNT_ALLOCS now writes into stderr, fix also tests for COUNT_ALLOCS #3910
  • [2.7] bpo-31692: Add PYTHONSHOWALLOCCOUNT env var #3927
  • Dependencies
  • bpo-23034: Dynamically control debugging output
  • Files
  • 00141-fix-tests_with_COUNT_ALLOCS.patch
  • 00141-fix-tests_with_COUNT_ALLOCS-v2.patch
  • 00141-fix-tests_with_COUNT_ALLOCS-v3.patch
  • 00141-fix-tests_with_COUNT_ALLOCS-v4.patch
  • 00141-fix-tests_with_COUNT_ALLOCS-v5.patch
  • 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/serhiy-storchaka'
    closed_at = <Date 2016-07-04.16:33:17.886>
    created_at = <Date 2013-11-08.12:30:13.712>
    labels = ['type-bug', 'tests']
    title = 'Test failures with COUNT_ALLOCS'
    updated_at = <Date 2017-10-17.09:25:27.987>
    user = 'https://bugs.python.org/bkabrda'

    bugs.python.org fields:

    activity = <Date 2017-10-17.09:25:27.987>
    actor = 'vstinner'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-07-04.16:33:17.886>
    closer = 'serhiy.storchaka'
    components = ['Tests']
    creation = <Date 2013-11-08.12:30:13.712>
    creator = 'bkabrda'
    dependencies = ['23034']
    files = ['32540', '33406', '37414', '37418', '37442']
    hgrepos = []
    issue_num = 19527
    keywords = ['patch']
    message_count = 19.0
    messages = ['202414', '202415', '207844', '207845', '207848', '232450', '232469', '232492', '232615', '255697', '255698', '255700', '255701', '255702', '255703', '255711', '269772', '269791', '304492']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'python-dev', 'serhiy.storchaka', 'bkabrda', 'rkuska']
    pr_nums = ['3910', '3927']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue19527'
    versions = ['Python 3.5', 'Python 3.6']

    @bkabrda
    Copy link
    Mannequin Author

    bkabrda mannequin commented Nov 8, 2013

    When Python is compiled with COUNT_ALLOCS, some tests in test_gc and test_module fail. I'm attaching the patch that skips 3 of them and modifies assertions in one of them, so that the tests pass.

    I'm however still unsure about one of the skipped tests, since I'm unsure whether I totally understand what's wrong there - test_gc_ordinary_module_at_shutdown.

    My guess is that due to COUNT_ALLOCS causing immortal types, the "final_a" and "final_b" types don't get destroyed on line [1] as they do in builds without COUNT_ALLOCS. AFAICS they are only "un-immortalized" on this line and destroyed during the following loop [2]. The problem here is that the order of destroyed modules is not deterministic, so sometimes the builtins module gets destroyed before the "final_X" and there is no "print" function, which makes the __del__ functions from "final_X" fail. IMO the best thing to do is to just skip this test with COUNT_ALLOCS. But I may be wrong, I don't have a great insight into Python's GC and module unloading.

    [1] http://hg.python.org/cpython/annotate/0f48843652b1/Python/import.c#l383
    [2] http://hg.python.org/cpython/annotate/0f48843652b1/Python/import.c#l394

    @bkabrda
    Copy link
    Mannequin Author

    bkabrda mannequin commented Nov 8, 2013

    And the patch...

    @bkabrda
    Copy link
    Mannequin Author

    bkabrda mannequin commented Jan 10, 2014

    Since 3.4.0.b2, this also causes failures in another tests: test_io, test_logging, test_threading, test_warnings. There are various cases testing that some types get collected when the interpreter shuts down.

    I'm attaching a new patch that covers all of these.

    @vstinner
    Copy link
    Member

    What is COUNT_ALLOCS?

    Python 3.4 now has sys.getallocatedblocks() and a new tracemalloc module which are compiled by default.

    @bkabrda
    Copy link
    Mannequin Author

    bkabrda mannequin commented Jan 10, 2014

    As noted in Misc/SpecialBuilds:

    COUNT_ALLOCS
    ------------

    Each type object grows three new members:

        /* Number of times an object of this type was allocated. */
        int tp_allocs;
    
        /* Number of times an object of this type was deallocated. */
        int tp_frees;
    
        /* Highwater mark:  the maximum value of tp_allocs - tp_frees so
         * far; or, IOW, the largest number of objects of this type alive at
         * the same time.
         */
        int tp_maxalloc;

    ...
    We use this for Fedora's python debug build to get some interesting debugging info. (If you try to "grep -r" through Python 3.4 source code, you'll find quite a few ifdefs with COUNT_ALLOCS.)

    @serhiy-storchaka serhiy-storchaka added tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Dec 10, 2014
    @serhiy-storchaka
    Copy link
    Member

    Why do you check hasattr(sys, 'getrefcount') in test_io.py, but hasattr(sys, 'getcounts') in all other tests?

    @bkabrda
    Copy link
    Mannequin Author

    bkabrda mannequin commented Dec 11, 2014

    Good catch, using getrefcount was a mistake. I'm attaching a new version which always checks for getcounts (and also applies on 3.4.2).

    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 11, 2014
    @serhiy-storchaka
    Copy link
    Member

    LGTM (only one nitpick -- there are trailing spaces in test_gc).

    But there are other tests which are failed with COUNT_ALLOCS. Here is a patch with additional fixes for these tests in test_gc and test_warnings.

    @serhiy-storchaka
    Copy link
    Member

    Thenks Antoine for great idea proposed in comments on Rietveld. Following patch introduces strip_python_stdout() which strips COUNT_ALLOCS debug output from stdout (unfortunately this operation is not always unambiguous) and call it in assert_python_ok() and assert_python_failure(). This automatically fixes a large number of tests. Also fixed a number of other tests failing with COUNT_ALLOCS. Virtually all tests are now fixed except test_doctest and test_distutils.

    A large part of the patch will be applied only to 3.4 and 2.7. With resolved bpo-23034 the patch for 3.5 will be much simpler, strip_python_stdout() and @requires_clean_stdout will gone away.

    @rkuska
    Copy link
    Mannequin

    rkuska mannequin commented Dec 2, 2015

    With Python-3.5 and COUNT_ALLOCS enabled following new tests fail also:

    FAIL: test_is_finalizing (test.test_sys.SysModuleTest)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/builddir/build/BUILD/Python-3.5.0/Lib/test/test_sys.py", line 767, in test_is_finalizing
        self.assertEqual(stdout.rstrip(), b'True')
    AssertionError: b'' != b'True'
    ---------------------------------
    
    

    ======================================================================
    FAIL: test_print_traceback_at_exit (test.test_traceback.SyntaxTracebackCases)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/builddir/build/BUILD/Python-3.5.0/Lib/test/test_traceback.py", line 210, in test_print_traceback_at_exit
        self.assertEqual(stderr.splitlines(), expected)
    AssertionError: Lists differ: [] != [b'Traceback (most recent call last):', b'[75 chars]ero']
    Second list contains 3 additional elements.
    First extra element 0:
    b'Traceback (most recent call last):'
    - []
    + [b'Traceback (most recent call last):',
    +  b'  File "<string>", line 8, in __init__',
    +  b'ZeroDivisionError: division by zero']

    @vstinner
    Copy link
    Member

    vstinner commented Dec 2, 2015

    COUNT_ALLOCS was added 22 years ago. I guess that the usecase is to track memory leaks, right?

    branch: legacy-trunk
    user: Sjoerd Mullender <sjoerd@acm.org>
    date: Mon Oct 11 12:54:31 1993 +0000
    files: Include/object.h Modules/arraymodule.c Modules/config.c.in Modules/imageop.c Modules/imgfile.c Objects/floatobject.c Objects/intobject.c Objects/listobj
    description:

    • Extended X interface: pixmap objects, colormap objects visual objects,
      image objects, and lots of new methods.
    • Added counting of allocations and deallocations of builtin types if
      COUNT_ALLOCS is defined. Had to move calls to NEWREF down in some
      files.
    • Bug fix in sorting lists.

    @vstinner
    Copy link
    Member

    vstinner commented Dec 2, 2015

    00141-fix-tests_with_COUNT_ALLOCS-v5.patch: please don't do that! It makes tests much more verbose for a compilation option which is hidden and probably almost never used in the wild. The option has no configuration option for example.

    *If* you really want to keep the feature, I would prefer to make it more visible (add a configuration option) and disable the output at exit by default. It's better to add a new "-X showcountallocs" as it was done with "-X showrefcount". Before, we had to fix a lot of unit tests (as 00141-fix-tests_with_COUNT_ALLOCS-v5.patch) to strip the "[xxx refs]" from stderr, it was very annoying.

    "Python 3.4 now has sys.getallocatedblocks() and a new tracemalloc module which are compiled by default."

    IMHO these two debug features superseded COUNT_ALLOCS. Please try to convince me of the use case of this very old debug feature.

    @rkuska
    Copy link
    Mannequin

    rkuska mannequin commented Dec 2, 2015

    FYI There is also bpo-23034 where is proposed "-X showalloccount" option to suppress the output, we use (custom patch) environment variable to filter out the verbose output in Fedora.

    @serhiy-storchaka
    Copy link
    Member

    Yes, I also don't want to use 00141-fix-tests_with_COUNT_ALLOCS-v5.patch if there is better alternative. See bpo-23034 (I'm uncertain only in option name).

    @bkabrda
    Copy link
    Mannequin Author

    bkabrda mannequin commented Dec 2, 2015

    IMHO these two debug features superseded COUNT_ALLOCS. Please try to convince me of the use case of this very old debug feature.

    I no longer use this feature and I think that noone does. As you said, it seems to be obsoleted by other new features, so my vote would be to remove COUNT_ALLOCS altogether.

    @vstinner
    Copy link
    Member

    vstinner commented Dec 2, 2015

    I propose to emit a compiler warning (or even an error?) in 3.5.x and drop
    the code in 3.6. I don't think that a long deprecation period is requied.
    The feature is not widely used.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 3, 2016

    New changeset 5abf6cdcac4d by Serhiy Storchaka in branch '3.5':
    Issue bpo-19527: Fixed tests with defined COUNT_ALLOCS.
    https://hg.python.org/cpython/rev/5abf6cdcac4d

    New changeset e7d84ecdd37d by Serhiy Storchaka in branch 'default':
    Issue bpo-19527: Fixed tests with defined COUNT_ALLOCS.
    https://hg.python.org/cpython/rev/e7d84ecdd37d

    @serhiy-storchaka
    Copy link
    Member

    All tests now are passed in 3.6 on Linux. Making them passing in 3.5 requires too much changes that are not needed in 3.6. I don't think we need to pollute tests with these temporary workarounds.

    @vstinner
    Copy link
    Member

    New changeset 7b4ba62 by Victor Stinner in branch '2.7':
    [2.7] bpo-31692: Add PYTHONSHOWALLOCCOUNT env var (GH-3927)
    7b4ba62

    @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
    tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants