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-45508: Specialize INPLACE_ADD by sweeneyde · Pull Request #29024 · python/cpython

Conversation

@sweeneyde

@sweeneyde sweeneyde commented Oct 18, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Pyperformance/Specialization results: https://gist.github.com/sweeneyde/41a76356e875e2a98d16ce5410ab41c0

https://bugs.python.org/issue45508

Sorry, something went wrong.

Uh oh!

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

All reactions
sweeneyde added 2 commits October 16, 2021 23:38
@sweeneyde
Initial implementation of INPLACE_MULTIPLY specialization
74ceb37
@sweeneyde
Merge branch 'main' into inplace_add
b366a06
@sweeneyde
sweeneyde requested a review from markshannon as a code owner October 18, 2021 09:50
@bedevere-bot bedevere-bot added the awaiting review label Oct 18, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 18, 2021
@blurb-it
📜🤖 Added by blurb_it.
62c1d38
@sweeneyde sweeneyde added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 18, 2021
@bedevere-bot

bedevere-bot commented Oct 18, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @sweeneyde for commit 62c1d38 🤖

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 Oct 18, 2021
@sweeneyde

sweeneyde commented Oct 18, 2021

Copy link
Copy Markdown
Member Author

The s390x RHEL7 LTO PR failure is https://bugs.python.org/issue45484

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Oct 18, 2021

Copy link
Copy Markdown
Member

You can remove unicode_concatenate

All reactions

Sorry, something went wrong.

Uh oh!

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

@sweeneyde
Remove unused unicode_concatenate
89ea6ba
@markshannon markshannon self-assigned this Oct 18, 2021
@markshannon

markshannon commented Oct 18, 2021

Copy link
Copy Markdown
Member

Any performance numbers or specialization stats for this?

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Oct 18, 2021

Copy link
Copy Markdown
Member

Maybe I should read the first comment 🙂

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Oct 18, 2021

Copy link
Copy Markdown
Member

I'd like to hold off merging this until we have a better way to handle classes like decimal.
I'll look into a fix for BINARY_ADD that we then apply here.

sweeneyde reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@corona10 corona10 mentioned this pull request Oct 18, 2021
bpo-45510: Specialize BINARY_SUBTRACT #29010
Closed
sweeneyde added 2 commits October 21, 2021 22:44
@sweeneyde
merge with main
48bdcf9
@sweeneyde
remove record_hit_inline calls
e4a1871
@sweeneyde

sweeneyde commented Oct 22, 2021

Copy link
Copy Markdown
Member Author

I merged with main and ran some microbenchmarks, and it seems decimal += decimal is not that bad.

Microbenchmark program:

from pyperf import Runner

runner = Runner()

runner.timeit("int+=int",
    setup="from itertools import repeat",
    stmt="x = 0\n"
         "for y in repeat(1, 10_000):\n"
         "    x += y; x += y; x += y; x += y; x += y"
    )
runner.timeit("float+=float",
    setup="from itertools import repeat",
    stmt="x = 0.0\n"
         "for y in repeat(1.0, 10_000):\n"
         "    x += y; x += y; x += y; x += y; x += y"
    )
runner.timeit("str+=str",
    setup="from itertools import repeat",
    stmt="for y in repeat('a', 10_000):\n"
         "    x = ''; x += y; x += y; x += y; x += y; x += y"
    )
runner.timeit("list[0]+=str",
    setup="from itertools import repeat",
    stmt="x = [None]\n"
         "for y in repeat('a', 10_000):\n"
         "    x[0] = ''; x[0] += y; x[0] += y; x[0] += y; x[0] += y; x[0] += y"
    )
runner.timeit("float+=int",
    setup="from itertools import repeat",
    stmt="x = 0.0\n"
         "for y in repeat(1, 10_000):\n"
         "    x += y; x += y; x += y; x += y; x += y"
    )
runner.timeit("decimal+=decimal",
    setup="from itertools import repeat; from decimal import Decimal as D",
    stmt="x = D(0)\n"
         "for y in repeat(D(1), 10_000):\n"
         "    x += y; x += y; x += y; x += y; x += y"
    )
runner.timeit("list[0]+=1",
    setup="from itertools import repeat; from collections import defaultdict",
    stmt="dd = [0]\n"
         "for y in repeat(1, 10_000):\n"
         "    dd[0] += y; dd[0] += y; dd[0] += y; dd[0] += y; dd[0] += y",
    )
runner.timeit("defaultdict(int)[0]+=1",
    setup="from itertools import repeat; from collections import defaultdict",
    stmt="dd = defaultdict(int)\n"
         "for y in repeat(1, 10_000):\n"
         "    dd[0] += y; dd[0] += y; dd[0] += y; dd[0] += y; dd[0] += y",
    )

Results from PGO on MSVC:

Benchmark main_inplace_add_micro specialized_inplace_add_micro
float+=float 1.18 ms 988 us: 1.19x faster
int+=int 1.34 ms 1.16 ms: 1.15x faster
str+=str 2.08 ms 1.80 ms: 1.15x faster
list[0]+=1 2.47 ms 2.39 ms: 1.03x faster
list[0]+=str 3.62 ms 3.67 ms: 1.01x slower
defaultdict(int)[0]+=1 3.29 ms 3.43 ms: 1.04x slower
float+=int 1.64 ms 1.74 ms: 1.06x slower
Geometric mean (ref) 1.05x faster

Benchmark hidden because not significant (1): decimal+=decimal

Results from PGO on GCC (WSL):

Benchmark main_inplace_add_micro2 specialized_inplace_add_micro2
float+=float 885 us 672 us: 1.32x faster
int+=int 1.14 ms 987 us: 1.16x faster
defaultdict(int)[0]+=1 2.61 ms 2.49 ms: 1.05x faster
list[0]+=1 2.03 ms 1.96 ms: 1.03x faster
float+=int 1.36 ms 1.32 ms: 1.03x faster
str+=str 1.39 ms 1.45 ms: 1.04x slower
list[0]+=str 2.75 ms 2.89 ms: 1.05x slower
Geometric mean (ref) 1.06x faster

Benchmark hidden because not significant (1): decimal+=decimal

corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Nov 10, 2021

Copy link
Copy Markdown
Member

Once #29482 is merged this will need to reworked for the Nth (and hopefully last) time. Sorry about that.
On the bright side, it should save some boilerplate.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sweeneyde

sweeneyde commented Nov 10, 2021

Copy link
Copy Markdown
Member Author

I think PR 29482 will just make this obsolete -- it manages to re-use *_ADD opcodes for INPLACE_ADD opcodes (nice!):

(In specialize.c)
        case NB_ADD:
        case NB_INPLACE_ADD:
            if (PyUnicode_CheckExact(lhs)) {
                if (_Py_OPCODE(instr[1]) == STORE_FAST && Py_REFCNT(lhs) == 2) {
                    *instr = _Py_MAKECODEUNIT(BINARY_OP_INPLACE_ADD_UNICODE,
                                              _Py_OPARG(*instr));
                    goto success;
                }
                *instr = _Py_MAKECODEUNIT(BINARY_OP_ADD_UNICODE,
                                          _Py_OPARG(*instr));
                goto success;
            }
All reactions

Sorry, something went wrong.

Uh oh!

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

@sweeneyde sweeneyde closed this Nov 11, 2021
@sweeneyde
sweeneyde deleted the inplace_add branch December 19, 2021 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

@markshannon markshannon

Labels

awaiting review

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.

4 participants

@sweeneyde @bedevere-bot @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.