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

Improve f-string implementation: FORMAT_VALUE opcode #69669

Closed
ericvsmith opened this issue Oct 26, 2015 · 18 comments
Closed

Improve f-string implementation: FORMAT_VALUE opcode #69669

ericvsmith opened this issue Oct 26, 2015 · 18 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ericvsmith
Copy link
Member

BPO 25483
Nosy @brettcannon, @larryhastings, @ericvsmith, @skrah, @markshannon, @berkerpeksag, @serhiy-storchaka
Files
  • format-opcode.diff
  • format-opcode-1.diff
  • format-opcode-2.diff
  • format-opcode-3.diff
  • 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 = 'https://github.com/ericvsmith'
    closed_at = <Date 2015-11-03.18:13:36.239>
    created_at = <Date 2015-10-26.15:44:59.106>
    labels = ['interpreter-core', 'type-feature']
    title = 'Improve f-string implementation: FORMAT_VALUE opcode'
    updated_at = <Date 2015-11-03.21:30:52.349>
    user = 'https://github.com/ericvsmith'

    bugs.python.org fields:

    activity = <Date 2015-11-03.21:30:52.349>
    actor = 'python-dev'
    assignee = 'eric.smith'
    closed = True
    closed_date = <Date 2015-11-03.18:13:36.239>
    closer = 'eric.smith'
    components = ['Interpreter Core']
    creation = <Date 2015-10-26.15:44:59.106>
    creator = 'eric.smith'
    dependencies = []
    files = ['40863', '40864', '40880', '40928']
    hgrepos = []
    issue_num = 25483
    keywords = ['patch']
    message_count = 18.0
    messages = ['253476', '253484', '253505', '253610', '253611', '253614', '253615', '253618', '253630', '253910', '253912', '253915', '253928', '254006', '254008', '254009', '254015', '254019']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'larry', 'eric.smith', 'skrah', 'Mark.Shannon', 'python-dev', 'berker.peksag', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25483'
    versions = ['Python 3.6']

    @ericvsmith
    Copy link
    Member Author

    Currently, the f-string f'a{3!r:10}' evaluates to bytecode that does the same thing as:

    ''.join(['a', format(repr(3), '10')])

    That is, it literally calls the functions format() and repr(). The same holds true for str() and ascii() with !s and !a, respectively.

    By redefining format, str, repr, and ascii, you can break or pervert the computation of the f-string's value:

    >>> def format(v, fmt=None): return '42'
    ...
    >>> f'{3}'
    '42'

    It's always been my intention to fix this. This patch adds an opcode FORMAT_VALUE, which instead of looking up format, etc., directly calls PyObject_Format, PyObject_Str, PyObject_Repr, and PyObject_ASCII. Thus, you can no longer modify what an f-string produces merely by overriding the named functions.

    In addition, because I'm now saving the name lookups and function calls, performance is improved.

    Here are the times without this patch:

    $ ./python -m timeit -s 'x="test"' 'f"{x}"'
    1000000 loops, best of 3: 0.3 usec per loop
    
    $ ./python -m timeit -s 'x="test"' 'f"{x!s}"'
    1000000 loops, best of 3: 0.511 usec per loop
    
    $ ./python -m timeit -s 'x="test"' 'f"{x!r}"'
    1000000 loops, best of 3: 0.497 usec per loop
    
    $ ./python -m timeit -s 'x="test"' 'f"{x!a}"'
    1000000 loops, best of 3: 0.461 usec per loop

    And with this patch:

    $ ./python -m timeit -s 'x="test"' 'f"{x}"'
    10000000 loops, best of 3: 0.02 usec per loop
    
    $ ./python -m timeit -s 'x="test"' 'f"{x!s}"'
    100000000 loops, best of 3: 0.02 usec per loop
    
    $ ./python -m timeit -s 'x="test"' 'f"{x!r}"'
    10000000 loops, best of 3: 0.0896 usec per loop
    
    $ ./python -m timeit -s 'x="test"' 'f"{x!a}"'
    10000000 loops, best of 3: 0.0923 usec per loop

    So a 90%+ speedup, for these simple cases.

    Also, now f-strings are faster than %-formatting, at least for some types:

    $ ./python -m timeit -s 'x="test"' '"%s"%x'
    10000000 loops, best of 3: 0.0755 usec per loop
    
    $ ./python -m timeit -s 'x="test"' 'f"{x}"'
    10000000 loops, best of 3: 0.02 usec per loop

    Note that people often "benchmark" %-formatting with code like the following. But the optimizer converts this to a constant string, so it's not a fair comparison:

    $ ./python -m timeit '"%s"%"test"'
    100000000 loops, best of 3: 0.0161 usec per loop

    These microbenchmarks aren't the end of the story, since the string concatenation also takes some time. That's another optimization I might implement in the future.

    Thanks to Mark and Larry for some advice on this.

    @ericvsmith ericvsmith self-assigned this Oct 26, 2015
    @ericvsmith ericvsmith added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Oct 26, 2015
    @ericvsmith
    Copy link
    Member Author

    Small cleanups. Fixed a bug in PyCompile_OpcodeStackEffect.

    @ericvsmith
    Copy link
    Member Author

    This patch addresses Larry's review, plus bumps the bytecode magic number.

    @ericvsmith
    Copy link
    Member Author

    Oops. Forgot to include the diff with that last message. But it turns out it doesn't work, anyway, because I put the #define's in opcode.h, which is generated (so my code got deleted!).

    I'll try to find some reasonable .h file to use and submit a new patch soon.

    @brettcannon
    Copy link
    Member

    I know this issue is slowly turning into "make Eric update outdated docs", but if you find that https://docs.python.org/devguide/compiler.html#introducing-new-bytecode is outdated, could you update that doc?

    @serhiy-storchaka
    Copy link
    Member

    I'll try to find some reasonable .h file to use and submit a new patch soon.

    It's Lib/opcode.py.

    @ericvsmith
    Copy link
    Member Author

    Brett: I'll take a look.

    Serhiy: I'm looking for a place to put some #defines related to the bit masks and bit values that my FORMAT_VALUE opcode is using for opargs. One option is to just put them in Tools/scripts/generate_opcode_h.py, so that they end up in the generated opcode.h, but that seems a little sleazy. I can't find a better place they'd belong, though.

    Specifically, I want to put these lines into a .h file to use by ceval.c and compile.c:

    /* Masks and values for FORMAT_VALUE opcode. */
    #define FVC_MASK      0x3
    #define FVS_MASK      0x4
    #define FVC_NONE      0x0
    #define FVC_STR       0x1
    #define FVC_REPR      0x2
    #define FVC_ASCII     0x3
    #define FVS_HAVE_SPEC 0x4

    @serhiy-storchaka
    Copy link
    Member

    Does the dis module need these constants? If no, you can use either ceval.h or compile.h.

    @ericvsmith
    Copy link
    Member Author

    Thanks, Serihy. I looked at those, and neither one is a great fit. But not having a better option, I went with ceval.h. Here's the patch.

    @ericvsmith
    Copy link
    Member Author

    Some formatting improvements.

    I removed one of the optimizations I was doing, because it's also done in PyObject_Format(). I plan on moving other optimizations into PyObject_Format(), but I'll open a separate issue for that.

    I swapped the order of the parameters on the stack, so that I could use the micro-optimization of TOP() and SET_TOP().

    I'll commit this shortly.

    @serhiy-storchaka
    Copy link
    Member

    It looks to me that FVS_MASK and FVS_HAVE_SPEC are duplicates. FVS_HAVE_SPEC is set but FVS_MASK is tested.

    @ericvsmith
    Copy link
    Member Author

    Right, they're the same because it's a single bit. You 'and' with a mask to get the bits you want, and you 'or' together the values. It's an old habit left over from my bit-twiddling days.

    I guess the test could really be:

    have_fmt_spec = (oparg & FVS_MASK) == FVS_HAVE_SPEC;

    to make it more clear what I'm doing.

    It's easier to see the same thing with the FVC_MASK and FVC_* values, since that field is multiple bits.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Nov 2, 2015

    The MASK idiom is nice and I think it's good to be exposed to
    it from time to time.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 3, 2015

    New changeset 1ddeb2e175df by Eric V. Smith in branch 'default':
    bpo-25483: Add an opcode to make f-string formatting more robust.
    https://hg.python.org/cpython/rev/1ddeb2e175df

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 3, 2015

    New changeset 4734713a31ed by Eric V. Smith in branch 'default':
    bpo-25483: Update dis.rst with FORMAT_VALUE opcode description.
    https://hg.python.org/cpython/rev/4734713a31ed

    @ericvsmith
    Copy link
    Member Author

    Brett: https://docs.python.org/devguide/compiler.html#introducing-new-bytecode looks correct (and reminded me to update dis.rst!).

    @berkerpeksag
    Copy link
    Member

      • (flags & 0x03) == 0x00: value is formattedd as-is.

    Just noticed a small typo: formattedd

    Also, needs .. versionadded:: 3.6

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 3, 2015

    New changeset 93fd7adbc7dd by Eric V. Smith in branch 'default':
    bpo-25483: Fix doc typo and added versionadded. Thanks, Berker Peksag.
    https://hg.python.org/cpython/rev/93fd7adbc7dd

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants