[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: I'm not aware of any pending issues, buildbots are happy, I'm happy, I close the issue :-) Don't hesitate to reopen it if I missed something. -- resolution: -> fixed stage: patch review -> resolved status: open -> closed

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: @Serhiy: Can we please now close this issue? Or is there still something to do? -- ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset e21cda70a3a13eb6e6238e436a5c0e2c4e4bebef by Serhiy Storchaka in branch 'master': Issue #29300: Use Argument Clinic for getting struct object from the format. https://github.com/python/cpython/commit/e21cda70a3a13eb6e6238e436a5c0e2c4e4bebef

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset 9245894af223 by Serhiy Storchaka in branch 'default': Issue #29300: Use Argument Clinic for getting struct object from the format. https://hg.python.org/cpython/rev/9245894af223 -- ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: cache_struct-2.patch LGTM. -- ___ Python tracker ___ ___ Python-bugs-list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Serhiy Storchaka: "Dear Roundup Robot, please don't close issues." For what it's worth, I reported issues of this robot to python-dev: https://mail.python.org/pipermail/python-dev/2017-February/147317.html

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: Dear Roundup Robot, please don't close issues. -- resolution: fixed -> stage: resolved -> patch review status: closed -> open Added file: http://bugs.python.org/file46492/cache_struct-2.patch ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Stefan Krah: "PyBUF_SIMPLE implies C-contiguous for a conforming buffer provider." Oh ok, thanks for the confirmation. So my change didn't add new checks and my commit message is wrong :-) Only non-conforming objects are impacted, but in such case, it's more

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Stefan Krah added the comment: PyBUF_SIMPLE implies C-contiguous for a conforming buffer provider. -- nosy: +skrah ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset a88eb4fa9e7fdf1a1050786223044c6bb7949784 by Victor Stinner in branch '3.6': Issue #29300: test_struct tests unpack_from() with keywords https://github.com/python/cpython/commit/a88eb4fa9e7fdf1a1050786223044c6bb7949784 --

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : -- resolution: -> fixed ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset a88eb4fa9e7fdf1a1050786223044c6bb7949784 by Victor Stinner in branch '3.5': Issue #29300: test_struct tests unpack_from() with keywords https://github.com/python/cpython/commit/a88eb4fa9e7fdf1a1050786223044c6bb7949784 --

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset a88eb4fa9e7fdf1a1050786223044c6bb7949784 by Victor Stinner in branch 'master': Issue #29300: test_struct tests unpack_from() with keywords https://github.com/python/cpython/commit/a88eb4fa9e7fdf1a1050786223044c6bb7949784 New changeset

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : -- status: open -> closed ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : -- stage: patch review -> resolved ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: I like the overall cache_struct.patch change, but I have questions: see my review. -- ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Martin Panter: """FYI Victor, you can make non-C-contiguous buffers by slicing memoryview: >>> struct.unpack(">L", memoryview(b"1234")[::-1]) Traceback (most recent call last): File "", line 1, in BufferError: memoryview: underlying buffer is not

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Serhiy Storchaka: "We already made such changes in the past. The difference is subtle and I have doubts that choosing any of ways was deliberate." Right. IMHO it's safe to make sure that the buffer is contiguous. I'm quite sure that the code doesn't support

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset faa1e4f4b156 by Victor Stinner in branch 'default': Rename struct.unpack() 2nd parameter to "buffer" https://hg.python.org/cpython/rev/faa1e4f4b156 -- ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset 32380d41e788 by Victor Stinner in branch '3.5': Issue #29300: test_struct tests unpack_from() with keywords https://hg.python.org/cpython/rev/32380d41e788 -- ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Serhiy Storchaka: "But after converting the struct module to Argument Clinic struct.pack() is faster than int.to_bytes() again!" Sorry about that ;-) Serhiy Storchaka: "Now I need to find other ways to make int.to_bytes() even faster to win this chase." I

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: unpack_buffer.patch LGTM. Please backport test changes and other changes discussed before to other branches. -- ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: > So yeah, the "side effect" is that struct.pack("i", 1) becomes 1.56x faster > (-36%). Ok, maybe it was my main goal ;-) I also mentioned the "new" (?) > contiguous requirement on buffers. struct.pack() always was faster than int.to_bytes(). I wanted to

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: > Oh by the way, I forgot to mention a subtle change. > PyObject_GetBuffer(PyBUF_SIMPLE) is less strict that PyArg_Parse("y#") / > "buffer" converter of Argument Clinic: getargs.c also checks that the > buffer is contiguous, extract of getbuffer(): We already

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Martin> Shouldn’t the top-level unpack() parameter be called “buffer” like the other functions and methods, not “inputstr”? Hum. I reopen the issue. Attached patch renames unpack() "inputstr" argument to "buffer" and uses the Py_buffer type for it. I had to

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Martin Panter added the comment: FYI Victor, you can make non-C-contiguous buffers by slicing memoryview: >>> struct.unpack(">L", memoryview(b"1234")[::-1]) Traceback (most recent call last): File "", line 1, in BufferError: memoryview: underlying buffer is not C-contiguous Can also use the

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: Propose for your consideration a patch that uses Argument Clinic for getting possible cached struct object from the format. This simplifies the implementation of module level functions. -- resolution: fixed -> stage: resolved -> patch review

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Martin Panter added the comment: Shouldn’t the top-level unpack() parameter be called “buffer” like the other functions and methods, not “inputstr”? -- nosy: +martin.panter ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : -- stage: -> resolved ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Changes by Roundup Robot : ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset e552e185f3087204d326409e8631eb33dd0e7958 by Victor Stinner in branch 'master': Issue #29300: Convert _struct module to Argument Clinic https://github.com/python/cpython/commit/e552e185f3087204d326409e8631eb33dd0e7958 --

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: I also prefer Serhiy's struct_fastcall-7.patch over my struct_fastcall-6.patch. Serhiy Storchaka: "This doesn't make the patch much bigger. Usually such kind of changes are made in one patch." Well, it's just that you prefer to reduce the number of commits

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Roundup Robot added the comment: New changeset f3ff4a3ce77c by Victor Stinner in branch 'default': Issue #29300: Convert _struct module to Argument Clinic https://hg.python.org/cpython/rev/f3ff4a3ce77c -- nosy: +python-dev ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: -if (PyObject_GetBuffer(input, , PyBUF_SIMPLE) < 0) -return NULL; Oh by the way, I forgot to mention a subtle change. PyObject_GetBuffer(PyBUF_SIMPLE) is less strict that PyArg_Parse("y#") / "buffer" converter of Argument Clinic: getargs.c also

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: This doesn't make the patch much bigger. Usually such kind of changes are made in one patch. Here is a patch that also changes the type of the self parameter to PyStructObject*. struct_fastcall-6.patch LGTM, but I prefer struct_fastcall-7.patch.

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: > It looks to me that the type of the self parameter can be changed from > PyObject* to PyStructObject*. This will make the patch larger but the final > code simpler. I like the idea, but I prefer to do in a separated change, once the first big one is

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: It looks to me that the type of the self parameter can be changed from PyObject* to PyStructObject*. This will make the patch larger but the final code simpler. class Struct "PyStructObject *" "" -- ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Serhiy: Would you mind to review my latest patch, struct_fastcall-6.patch? It should address all your remarks, except your proposal to write a converter for cache_struct(): see my previous comment about this idea. Thanks in advance ;) --

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Updated patch, version 6: * Rename "obj" variable to "iter" * Fix unpack_from() signature: only the first parameter is positional only -- Added file: http://bugs.python.org/file46436/struct_fastcall-6.patch ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: New patch version 5 without the "format_converter". Serhiy Storchaka: I said not about the converter that converts format to bytes (it is needed only in Struct.__init__), For me, it makes sense to check format type earlier than Struct.__init__(), but since

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: I said not about the converter that converts format to bytes (it is needed only in Struct.__init__), but about the converter that converts format to the Struct object. It is used in all module-level functions. --

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: > See also ascii_buffer_converter in Modules/binascii.c for example. Nice. I didn't know the "def cleanup" feature. I used it in my patch version 4. -- ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Thank you for your reviews Serhiy :-) Here is a new patch to take in account your latest comments. I didn't change the API anymore: Struct.unpack_from() accepts keywords, unpack_from() doesn't. -- Added file:

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: See also ascii_buffer_converter in Modules/binascii.c for example. -- ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: static int cache_struct_converter(PyObject *arg, PyObject **s_object) { if (arg == NULL) { Py_DECREF(); return 1; } *s_object = cache_struct(arg); // actually inline this if (*s_object == NULL) return 0; return

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: > The converter for the format parameter already exists. It is cache_struct(). > You just need to change its interface. The code to convert the format string is in s_init(). The code increases the reference counter, I don't see how to produce the Py_DECREF()

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: The converter for the format parameter already exists. It is cache_struct(). You just need to change its interface. > * Fix Struct.__init__() error message: the method accepts also Unicode > * Document that Struct accepts str encodable to ASCII and bytes I

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Patch version 3: * Use also Argument Clinic for Struct.__init__() * Rename fmt to format: in the code and docstring. By the way, Struct docstring was wrong: Struct() accepts a 'format' keyword argument, not 'fmt' * Use Py_buffer converter for the buffer

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: It looks to me that the Py_buffer converter can be used for all the buffer parameter and special purposed converter can be used for the fmt parameter. -- ___ Python tracker

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: I reformatted docstrings, and wrote a summary line. -- ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

STINNER Victor added the comment: Patch version 2 converts most functions and methods to Argument Clinic, except of unpack() and unpack_into() which require "*args" support in Argument Clinic: see the issue #20291. -- Added file:

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

Serhiy Storchaka added the comment: Why not just convert the struct module to Argument Clinic? -- ___ Python tracker ___

[issue29300] Modify the _struct module to use FASTCALL and Argument Clinic

New submission from STINNER Victor: Attached patch modify the _struct module to use FASTCALL and Argument Clinic. AC requires a summary line, so I duplicated the first sentence of iter_unpack() docstring: iter_unpack(fmt, buffer, /) Return an iterator yielding tuples unpacked from the