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

Add vectorcall for generic alias object #91223

Closed
penguin-wwy mannequin opened this issue Mar 19, 2022 · 9 comments
Closed

Add vectorcall for generic alias object #91223

penguin-wwy mannequin opened this issue Mar 19, 2022 · 9 comments
Labels
3.11 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir topic-typing

Comments

@penguin-wwy
Copy link
Mannequin

penguin-wwy mannequin commented Mar 19, 2022

BPO 47067
Nosy @gvanrossum, @JelleZijlstra, @corona10, @sweeneyde, @Fidget-Spinner, @penguin-wwy
PRs
  • bpo-47067: Add vectorcall for gaobject #31996
  • 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 = None
    created_at = <Date 2022-03-19.15:31:34.611>
    labels = ['library', '3.11']
    title = 'Add vectorcall for generic alias object'
    updated_at = <Date 2022-03-21.21:15:20.006>
    user = 'https://github.com/penguin-wwy'

    bugs.python.org fields:

    activity = <Date 2022-03-21.21:15:20.006>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2022-03-19.15:31:34.611>
    creator = 'penguin_wwy'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 47067
    keywords = ['patch']
    message_count = 8.0
    messages = ['415556', '415589', '415613', '415615', '415616', '415630', '415656', '415699']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'JelleZijlstra', 'corona10', 'Dennis Sweeney', 'kj', 'penguin_wwy']
    pr_nums = ['31996']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue47067'
    versions = ['Python 3.11']

    @penguin-wwy
    Copy link
    Mannequin Author

    penguin-wwy mannequin commented Mar 19, 2022

    Although ga_call determines whether origin has a vectorcall, it needs to be unpacked the parameters that are already packed.
    /-> origin.vectorcall(unpacked)
    MakeTpCall(packed) -> ga_call -> PyObject_Call
    -> origin.tp_call

    We can advance the vectorcall judgment to the setup phase.

    ga_vectorcall -> origin.vectorcall

    or

    ga_make_tp_call -> _PyObject_MakeTpCall(packed argument) -> origin.tp_call

    This will have no effect on tp_call, which still only needs to be packed once, while vectorcall does not need packed/unpacked

    @penguin-wwy penguin-wwy mannequin added 3.11 only security fixes stdlib Python modules in the Lib dir labels Mar 19, 2022
    @corona10
    Copy link
    Member

    We decided not to add it
    see bpo-40369

    @penguin-wwy
    Copy link
    Mannequin Author

    penguin-wwy mannequin commented Mar 20, 2022

    The point of bpo-40369 issue seems to be to provide vectorcall to the GenericAliasType, means vectorcall for Queue[int]

    However, my idea is to add vectorcall to gaobject, like this:

    d = dict[int]
    d(a=1, b=2) <-- vectorcall
    

    The idea came from my desire to implement different subclasses of a type when implementing some kind of factory model:

    class ChildClassBuilder:
        ...
    
        def set_type(self, t: Type):
            self.t = t
    
        def build():
            instance = self.t()
            ...
            return instance
    

    In the case of high type-hint coverage, the self.t is often a gaobject.

    Benchmark example:

    def test():
        d = dict[int]
        for _ in range(1000000):
            d(a=1, b=2)
    

    Speedup 20% in my wsl2

    opt:
       ncalls  tottime  percall  cumtime  percall filename:lineno(function)
            1    0.000    0.000    1.728    1.728 <string>:1(<module>)
            1    1.728    1.728    1.728    1.728 test.py:4(test)
    
    master:
       ncalls  tottime  percall  cumtime  percall filename:lineno(function)
            1    0.000    0.000    2.271    2.271 <string>:1(<module>)
            1    2.271    2.271    2.271    2.271 test.py:4(test)
    

    @penguin-wwy
    Copy link
    Mannequin Author

    penguin-wwy mannequin commented Mar 20, 2022

    There is a small problem with the example:

    d = dict[int]
    

    should

    d = dict[str, int]
    

    @gvanrossum
    Copy link
    Member

    @dennis, if/when the PR looks good to you, you can merge it.

    @sweeneyde
    Copy link
    Member

    I profiled dict[str, int](a=1, b=2), and it looks like a decent chunk of time comes from PyUnicode_New as used by PyObject_SetAttrString.

    You could also try replacing PyObject_SetAttrString with PyObject_SetAttr and adding "__orig_class__" to the global strings with Tools/scripts/generate_global_objects.py, probably for a later PR.

    @penguin-wwy
    Copy link
    Mannequin Author

    penguin-wwy mannequin commented Mar 21, 2022

    You could also try replacing PyObject_SetAttrString with PyObject_SetAttr and adding "__orig_class__" to the global strings with Tools/scripts/generate_global_objects.py, probably for a later PR.

    Already done via make regen-global-objects

    Also, I found that regen-global-objects depends on regen-deepfreeze, which should be a bug. This causes the regen-global-objects to be executed with the compile task first, and it does not compile successfully at this point because of the new symbols(_Py_ID(orig_class)) added.

    @sweeneyde
    Copy link
    Member

    New changeset 1ea055b by penguin_wwy in branch 'main':
    bpo-47067: Optimize calling GenericAlias objects (GH-31996)
    1ea055b

    @gvanrossum gvanrossum changed the title Add vectorcall for generica alias object Add vectorcall for generic alias object Mar 21, 2022
    @gvanrossum gvanrossum changed the title Add vectorcall for generica alias object Add vectorcall for generic alias object Mar 21, 2022
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @AlexWaygood AlexWaygood added performance Performance or resource usage topic-typing labels Apr 13, 2022
    @kumaraditya303
    Copy link
    Contributor

    Closing as the associated PR is merged.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir topic-typing
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants