GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44953: Add vectorcall for itemgetter and attrgetter by sweeneyde · Pull Request #27828 · python/cpython

Conversation

@sweeneyde

@sweeneyde sweeneyde commented Aug 19, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue44953

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sweeneyde added 2 commits August 19, 2021 03:55
@sweeneyde
Add vectorcall for itemgetter objects
f8e7d88
@sweeneyde
Add vectorcall for attrgetter objects
4033d27
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 19, 2021
@bedevere-bot bedevere-bot added the awaiting review label Aug 19, 2021
@blurb-it
📜🤖 Added by blurb_it.
f9fc83a
@sweeneyde

sweeneyde commented Aug 19, 2021

Copy link
Copy Markdown
Member Author

This was inspired by GH-27782, which does the same, and more, for methodcaller.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sweeneyde

sweeneyde commented Aug 19, 2021

Copy link
Copy Markdown
Member Author

Benchmarks for this change:

itemgetter: Mean +- std dev: [operator_main] 45.3 ns +- 1.3 ns -> [operator_vec] 29.5 ns +- 0.7 ns: 1.54x faster
attrgetter: Mean +- std dev: [operator_main] 61.6 ns +- 1.7 ns -> [operator_vec] 43.8 ns +- 0.9 ns: 1.41x faster
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sweeneyde
sweeneyde requested a review from markshannon August 19, 2021 09:40
rhettinger
rhettinger approved these changes Aug 20, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 20, 2021
@rhettinger rhettinger added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 20, 2021
@bedevere-bot

bedevere-bot commented Aug 20, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @rhettinger for commit f9fc83a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 20, 2021
markshannon
markshannon reviewed Aug 20, 2021
View reviewed changes
Comment thread Modules/_operator.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

markshannon
markshannon reviewed Aug 20, 2021
View reviewed changes
Comment thread Modules/_operator.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@github-actions

github-actions Bot commented Sep 20, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Sep 20, 2021
@sweeneyde

sweeneyde commented Oct 2, 2021

Copy link
Copy Markdown
Member Author

The __vectorcalloffset__ behavior was added in GH-20026. I don't think I understand that design decision rather than adding a new PyType_Slot.slot value called something like Py_tp_vectorcall, but it appears to be what we have. The corresponding behavior of __dictoffset__ and __weaklistoffset__ goes back to 2019 in GH-16076. Are there any plans to change these, or is there anything holding this up?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Oct 4, 2021
methane
methane approved these changes Dec 24, 2021
View reviewed changes
@sweeneyde
Merge remote-tracking branch 'upstream/main' into itemgetter_vectorcall
429f039
@sweeneyde

sweeneyde commented Jan 3, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Any plans for this? It may also be relevant to bpo-46148 about optimizing Pathlib.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sweeneyde

sweeneyde commented Jan 23, 2022

Copy link
Copy Markdown
Member Author

Does anyone with a commit bit want to merge this? This is the kind of callable that's often called in loops.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sweeneyde
Catch up with main
3c0fa46
@sweeneyde

sweeneyde commented Feb 10, 2022

Copy link
Copy Markdown
Member Author

I re-ran some benchmarks, and they still look good. I plan to merge this in about 24 hours if there are no more objections.

from operator import itemgetter, attrgetter
from itertools import repeat
from collections import namedtuple, deque
from pyperf import Runner

class DefaultClass:
    def __init__(self, a, b):
        self.a = a
        self.b = b

class SlotsClass:
    __slots__ = "a", "b"
    def __init__(self, a, b):
        self.a = a
        self.b = b

NamedTuple = namedtuple("NT", ["a", "b"])
MAP_LOOPS = 10_000

attr_classes = {
    'DefaultClass': DefaultClass,
    'SlotsClass': SlotsClass,
    'NamedTuple': NamedTuple,
}

item_classes = {
    'tuple': tuple,
    'list': list,
    'dict': dict.fromkeys,
}

namespace = {
    'IG': itemgetter(1),
    'AG': attrgetter('a'),
    'repeat': repeat,
    'deque': deque,
} | attr_classes | item_classes

runner = Runner()

for classname in attr_classes:
    runner.timeit(
        name=f"{classname}-1",
        setup=f"obj = {classname}(11, 22)",
        stmt="AG(obj)",
        globals=namespace
    )
    runner.timeit(
        name=f"{classname}-map",
        setup=f"obj = {classname}(11, 22)",
        stmt=f"deque(map(AG, repeat(obj, {MAP_LOOPS})), maxlen=0)",
        globals=namespace,
        inner_loops=MAP_LOOPS,
    )

for classname in item_classes:
    runner.timeit(
        name=f"{classname}-1",
        setup=f"obj = {classname}((1, 2, 3, 4, 5))",
        stmt="IG(obj)",
        globals=namespace,
    )
    runner.timeit(
        name=f"{classname}-map",
        setup=f"obj = {classname}((1, 2, 3, 4, 5))",
        stmt=f"deque(map(IG, repeat(obj, {MAP_LOOPS})), maxlen=0)",
        globals=namespace,
        inner_loops=MAP_LOOPS,
    )

Results:

Benchmark main_bench PR-27828
tuple-map 27.6 ns 13.2 ns: 2.09x faster
dict-map 39.4 ns 24.2 ns: 1.62x faster
NamedTuple-map 40.0 ns 24.8 ns: 1.61x faster
list-map 38.2 ns 23.8 ns: 1.61x faster
SlotsClass-map 42.8 ns 28.1 ns: 1.53x faster
DefaultClass-map 45.5 ns 31.0 ns: 1.47x faster
tuple-1 56.7 ns 41.4 ns: 1.37x faster
NamedTuple-1 70.1 ns 52.7 ns: 1.33x faster
dict-1 69.4 ns 52.2 ns: 1.33x faster
SlotsClass-1 75.1 ns 56.6 ns: 1.33x faster
list-1 68.7 ns 52.1 ns: 1.32x faster
DefaultClass-1 76.9 ns 59.4 ns: 1.29x faster
Geometric mean (ref) 1.48x faster
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sweeneyde
sweeneyde merged commit 0a14506 into python:main Feb 10, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 10, 2022
@sweeneyde
sweeneyde deleted the itemgetter_vectorcall branch February 10, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon left review comments
@methane methane methane approved these changes
@rhettinger rhettinger rhettinger approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@sweeneyde @bedevere-bot @methane @rhettinger @markshannon @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.