Skip to content

bpo-47053: Refactor BINARY_OP_INPLACE_ADD_UNICODE #32122

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

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

sweeneyde
Copy link
Member

@sweeneyde sweeneyde commented Mar 26, 2022

Letting PyUnicode_Append directly modify the target local avoids the next STORE_FAST, and it avoids the GETLOCAL(next_oparg) = NULL;.

https://bugs.python.org/issue47053

@sweeneyde
Copy link
Member Author

sweeneyde commented Mar 26, 2022

Microbenchmark:

./python -m pyperf timeit -s "r = 'a'*10_000; s=''" "for c in r: s += c"

Mean +- std dev: [before] 454 us +- 3 us -> [after] 437 us +- 4 us: 1.04x faster

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

PyObject *var = GETLOCAL(next_oparg);
DEOPT_IF(var != left, BINARY_OP);
PyObject **target_local = &GETLOCAL(_Py_OPARG(true_next));
DEOPT_IF(*target_local != left, BINARY_OP);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the last iteration here for example:

>>> pairs = [('foo',) * 2 for _ in range(10_000)]
>>> pairs.append(('foo', 'bar'))
>>> def f():
...     for x, y in pairs:
...         x = y + 'a'
...
>>> f()

@sweeneyde sweeneyde merged commit 7881549 into python:main Mar 29, 2022
@sweeneyde sweeneyde deleted the inplace_add_unicode_refactor branch March 29, 2022 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants