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

Implement ABCMeta in C #75514

Closed
ilevkivskyi opened this issue Sep 3, 2017 · 18 comments
Closed

Implement ABCMeta in C #75514

ilevkivskyi opened this issue Sep 3, 2017 · 18 comments
Labels
3.7 3.8 deferred-blocker extension-modules performance stdlib

Comments

@ilevkivskyi
Copy link
Contributor

@ilevkivskyi ilevkivskyi commented Sep 3, 2017

BPO 31333
Nosy @gvanrossum, @warsaw, @brettcannon, @terryjreedy, @scoder, @vstinner, @ned-deily, @methane, @ericsnowcurrently, @serhiy-storchaka, @1st1, @ilevkivskyi, @aaronchall, @miss-islington
PRs
  • #5273
  • #5733
  • #5744
  • #5745
  • 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 = None
    closed_at = <Date 2018-02-18.12:49:52.291>
    created_at = <Date 2017-09-03.20:54:21.774>
    labels = ['deferred-blocker', '3.8', 'performance', 'extension-modules', '3.7', 'library']
    title = 'Implement ABCMeta in C'
    updated_at = <Date 2018-02-18.22:35:42.221>
    user = 'https://github.com/ilevkivskyi'

    bugs.python.org fields:

    activity = <Date 2018-02-18.22:35:42.221>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-02-18.12:49:52.291>
    closer = 'levkivskyi'
    components = ['Extension Modules', 'Library (Lib)']
    creation = <Date 2017-09-03.20:54:21.774>
    creator = 'levkivskyi'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31333
    keywords = ['patch']
    message_count = 18.0
    messages = ['301198', '301236', '301241', '301290', '301293', '301301', '301302', '310998', '310999', '312284', '312285', '312287', '312306', '312315', '312323', '312327', '312329', '312331']
    nosy_count = 14.0
    nosy_names = ['gvanrossum', 'barry', 'brett.cannon', 'terry.reedy', 'scoder', 'vstinner', 'ned.deily', 'methane', 'eric.snow', 'serhiy.storchaka', 'yselivanov', 'levkivskyi', 'Aaron Hall', 'miss-islington']
    pr_nums = ['5273', '5733', '5744', '5745']
    priority = 'deferred blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue31333'
    versions = ['Python 3.7', 'Python 3.8']

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Sep 3, 2017

    The idea is that creating ABCs is approximately twice slower than normal classes. Plus there are smaller penalties for various operations with ABCs. This mostly influences the Python interpreter start-up time (because of extensive use of ABCs in importlib), and start-up times of programs that extensively use ABCs.

    The situation can be improved by rewriting ABCMeta in C. I have a working implementation, but it is far form being ready and still needs some polishing and optimizations (in particular _abc_cache and friends).

    Already at this stage I have one question (I will add more when they appear while I am finishing the implementation): is it OK to make _abc_cache, _abc_negative_cache, _abc_negative_cache_version, and _abc_registry read-only? The point is that I want to prohibit something like this:

        MyABC._abc_cache = "Surprise on updating the cache!"

    thus avoiding many PySet_Check(...) calls. These attributes are not documented and start with underscore.

    @ilevkivskyi ilevkivskyi added 3.7 extension-modules stdlib performance labels Sep 3, 2017
    @ericsnowcurrently
    Copy link
    Member

    @ericsnowcurrently ericsnowcurrently commented Sep 4, 2017

    This mostly influences the Python interpreter start-up time
    (because of extensive use of ABCs in importlib)

    Just to be clear, the only ABCs in importlib are in importlib.abc (and used by importlib.util). This does not impact interpreter startup.

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Sep 4, 2017

    Eric,

    the only ABCs in importlib are in importlib.abc (and used by importlib.util).
    This does not impact interpreter startup.

    Hm, indeed, but I see that module 'abc' is in 'sys.modules', probably it is then only used by 'collections.abc'/'_collections_abc'. This means that the influence of 'ABCMeta' on interpreter start-up time will be smaller than I thought. But still start-up times for projects that actively use ABCs will be influenced by 'ABCMeta'.

    But what do you think about making private ABC caches read-only attributes? (They are not documented)

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Sep 5, 2017

    I've heard many anecdotal complaints about the lack of speed of ABCs. Even if it doesn't affect core Python startup, it does affect startup of apps, and if people are afraid to use them because of this, it's reasonable to try to do something about it.

    @methane
    Copy link
    Member

    @methane methane commented Sep 5, 2017

    Hm, indeed, but I see that module 'abc' is in 'sys.modules', probably it is then only used by 'collections.abc'/'_collections_abc'. This means that the influence of 'ABCMeta' on interpreter start-up time will be smaller than I thought. But still start-up times for projects that actively use ABCs will be influenced by 'ABCMeta'.

    Since os.environ uses _collections_abc.MutableMapping, importing _collections_abc is not avoidable while startup.

    io module uses ABC and it's used for sys.std(io|err|in). It's not avoidable too.

    And as you know, typing module will be very common rapidly :)
    Even if it doesn't affect "python_startup" microbench, it's important.

    @scoder
    Copy link
    Contributor

    @scoder scoder commented Sep 5, 2017

    Since the number of applications that get along without any file access is probably close to irrelevant, "os" and "io" feel like sufficiently widely used modules to merit being part of a "usual Python startup" benchmark. Maybe we should add one to the benchmark suite? Anyone up for it?

    @methane
    Copy link
    Member

    @methane methane commented Sep 5, 2017

    "python_startup_nosite" benchmark imports io module,
    and "python_startup" benchmark imports os too.
    So no need to additional test for them.

    You can see it with python -v -S -c 'pass' and python -v -c 'pass'.

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 28, 2018

    There has been a lot of work by a number of people on this feature leading up to the 3.7.0b1. Thank you! On PR 5273, Yury says that he believes the feature is ready to merge but would prefer an extension until 3.7.0b2 and that Guido has agreed. I'll defer to Yury's judgement on this and somewhat reluctantly allow an extension: there's a lot going on here. Let's try to get it in as soon as we can, please!

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 28, 2018

    Let's try to get it in as soon as we can, please!

    Thank you, Ned! We'll get it merged in the next few days.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Feb 17, 2018

    Isn't 800 lines of C code too high price for speeding up ABCs creation? This will save several microseconds per ABC creation. Even if the program creates 1000 ABCs, this can save just several milliseconds at start-up.

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Feb 17, 2018

    Isn't 800 lines of C code too high price for speeding up ABCs creation?

    800 lines of C code is not something hard to notice, so I suppose the answer is obvious for all people involved in the work on PR :-)

    ...this can save just several milliseconds at start-up.

    The correct way to measure this is relative, not absolute. There are just few ABCs used by modules loaded at Python start-up, and it already allowed to save 10% of start-up time. My expectation is that the number will be similar for a typical Python app. Moreover, isinstance and issubclass (functions called often with ABCs) will be 1.5x faster.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Feb 17, 2018

    Isn't 800 lines of C code too high price for speeding up ABCs creation?

    I think it's well worth it. This has long felt as a sore point to me.

    On Sat, Feb 17, 2018 at 10:27 AM, Ivan Levkivskyi <report@bugs.python.org>
    wrote:

    Ivan Levkivskyi levkivskyi@gmail.com added the comment:

    Isn't 800 lines of C code too high price for speeding up ABCs creation?

    800 lines of C code is not something hard to notice, so I suppose the
    answer is obvious for all people involved in the work on PR :-)

    ...this can save just several milliseconds at start-up.

    The correct way to measure this is relative, not absolute. There are just
    few ABCs used by modules loaded at Python start-up, and it already allowed
    to save 10% of start-up time. My expectation is that the number will be
    similar for a typical Python app. Moreover, isinstance and issubclass
    (functions called often with ABCs) will be 1.5x faster.

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue31333\>


    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Feb 18, 2018

    New changeset 03e3c34 by Ivan Levkivskyi in branch 'master':
    bpo-31333: Re-implement ABCMeta in C (bpo-5273)
    03e3c34

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Feb 18, 2018

    New changeset 3892899 by Ivan Levkivskyi in branch '3.7':
    bpo-31333: Re-implement ABCMeta in C (GH-5733)
    3892899

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Feb 18, 2018

    Congratulations all on this milestone! And thanks reviewers for your thorough work.

    @terryjreedy
    Copy link
    Member

    @terryjreedy terryjreedy commented Feb 18, 2018

    PR fixes typo in What's new: rewrittent

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Feb 18, 2018

    New changeset 3fb813d by Ivan Levkivskyi (Terry Jan Reedy) in branch 'master':
    bpo-31333: Fix typo in whatsnew/3.7.rst (GH-5744)
    3fb813d

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Feb 18, 2018

    New changeset 034a945 by Miss Islington (bot) in branch '3.7':
    bpo-31333: Fix typo in whatsnew/3.7.rst (GH-5744)
    034a945

    @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
    3.7 3.8 deferred-blocker extension-modules performance stdlib
    Projects
    None yet
    Development

    No branches or pull requests

    10 participants