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

[STINNER Victor]

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

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor added the comment:

@Serhiy: Can we please now close this issue? Or is there still something to do?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor added the comment:

cache_struct-2.patch LGTM.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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

https://mail.python.org/pipermail/python-dev/2017-February/147325.html

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 a bug in the object than a bug in struct. I guess that 
non-conforming non-contiguous objects are very rare in the wild ;-)

The new code is more strict, so is safe in all corner cases.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Stefan Krah]

Stefan Krah added the comment:

PyBUF_SIMPLE implies C-contiguous for a conforming buffer provider.

--
nosy: +skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


--
resolution:  -> fixed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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 dcd4b1af2c59b0aae33cbac00d9f6fb47782ac57 by Victor Stinner in 
branch 'master':
Rename struct.unpack() 2nd parameter to "buffer"
https://github.com/python/cpython/commit/dcd4b1af2c59b0aae33cbac00d9f6fb47782ac57


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


--
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


--
stage: patch review -> resolved

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor added the comment:

I like the overall cache_struct.patch change, but I have questions: see my 
review.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 C-contiguous"""

Oh, it means that the Argument Clinic change doesn't add new checks on the 
buffer? In Python 3.6, memory_getbuf() raises an exception in the following 
code:

if (!REQ_STRIDES(flags)) {
if (!MV_C_CONTIGUOUS(baseflags)) {
PyErr_SetString(PyExc_BufferError,
"memoryview: underlying buffer is not C-contiguous");
return -1;
}
view->strides = NULL;
}

I undersrtand that memory_getbuf() is smart enough to raise an exception 
becaues the buffer is not contiguous. But a weaker implementation of getbuffer 
may not implement such check, whereas getbuffer() double check that the buffer 
is C-contiguous. Am I right?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 non-contiguous buffers.


Serhiy Storchaka: "Please backport test changes and other changes discussed 
before to other branches."

Done. Sorry, I forgot this part.


Serhiy Storchaka: "unpack_buffer.patch LGTM."

Thanks for the review, it's now merged. It was a regression in unpack() 
docstring, Python 3.5 docstring contains "unpack(fmt, buffer)".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 ran a microbenchmark:

$ ./python -m perf timeit -s 'to_bytes=int.to_bytes' 'to_bytes(1, 4, "little")'

Reference: ~154 ns

Replace int_to_bytes_impl() body with:
   PyBytes_FromStringAndSize("1", 1)
=> ~120 ns (-34 ns)

Replace int_to_bytes() body with:
   return int_to_bytes_impl(self, 4, NULL, 1);
=> ~76 ns (-44 ns)

_PyArg_ParseStackAndKeywords() with _PyArg_Parser{"nU|$p:to_bytes"} takes 44 ns 
on a total of 154 ns. 29% of the runtime is spent on parsing arguments.

If you want to optimize further int.to_bytes(), IMHO we should explore the 
issue #29419: "Argument Clinic: inline PyArg_UnpackTuple and 
PyArg_ParseStack(AndKeyword)?".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

unpack_buffer.patch LGTM. Please backport test changes and other changes 
discussed before to other branches.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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 speed up 
int.to_bytes(), and after converting to Argument Clinic in issue20185 it have 
became faster than struct.pack(). But after converting the struct module to 
Argument Clinic struct.pack() is faster than int.to_bytes() again! Now I need 
to find other ways to make int.to_bytes() even faster to win this chase.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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 made such changes in the past. The difference is subtle and I have 
doubts that choosing any of ways was deliberate.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 fix unit tests which passed str instead of 
bytes to buffer. Before this error was missed because the unit test is written 
to test the format string value, not the type of arguments.

--
resolution: fixed -> 
Added file: http://bugs.python.org/file46490/unpack_buffer.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

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 built-in _testbuffer module to create stranger buffers.

--
resolution:  -> fixed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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
status: closed -> open
Added file: http://bugs.python.org/file46489/cache_struct.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


--
stage:  -> resolved

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Changes by Roundup Robot :


___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 and so fold 
tiny changes into a single big commit, whereas I now prefer multiple tiny 
commits. I have not strong preferences between the two ways to commit, so I 
pushed your patch.

Here is the full commit message, since Roundbot bot only shows the first line:
---
Issue #29300: Convert _struct module to Argument Clinic

* The struct module now requires contiguous buffers.
* Convert most functions and methods of the _struct module to Argument Clinic
* Use "Py_buffer" type for the "buffer" argument. Argument Clinic is
  responsible to create and release the Py_buffer object.
* Use "PyStructObject *" type for self to avoid explicit conversions.
* Add an unit test on the _struct.Struct.unpack_from() method to test passing
  arguments as keywords.
* Rephrase docstrings.
* Rename "fmt" argument to "format" in docstrings and the documentation.

As a side effect, functions and methods which used METH_VARARGS calling
convention like struct.pack() now use the METH_FASTCALL calling convention
which avoids the creation of temporary tuple to pass positional arguments and
so is faster. For example, struct.pack("i", 1) becomes 1.56x faster (-36%)::

$ ./python -m perf timeit \
-s 'import struct; pack=struct.pack' 'pack("i", 1)' \
--compare-to=../default-ref/python
Median +- std dev: 119 ns +- 1 ns -> 76.8 ns +- 0.4 ns: 1.56x faster (-36%)
Significant (t=295.91)

Patch co-written with Serhiy Storchaka.
---

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.

Thanks Serhiy for your multiple reviews, very useful as usual. I like the final 
result: better documentation, better docstrings and better code!

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 checks that the buffer is 
contiguous, extract of getbuffer():

if (!PyBuffer_IsContiguous(view, 'C')) {
PyBuffer_Release(view);
*errmsg = "contiguous buffer";
return -1;
}

I don't know well the buffer protocol. I don't know any object which provide a 
non-contiguous buffer. At least, I can say that the last time I looked at this 
dark part of Python, the documentation was between tiny and non-existent :-/ 
The buffer protocol is complex but not well documented :-(

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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.

--
Added file: http://bugs.python.org/file46488/struct_fastcall-7.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 merged. Otherwise, the first patch will do too many unrelated things at 
the same time IMHO.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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 *" ""

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 ;)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 you like the current code, I'm also ok to leave this part of the code 
unchanged :-)


Serhiy: but about the converter that converts format to the Struct object. It 
is used in all module-level functions.

Sorry, but I dislike this change. Even if multiple functions do the same, for 
me creating a Struct object is not something part of the argument parsing. I 
prefer to keep the following 3 lines in each C function *body* to ease 
debugging:

PyObject *s_object = cache_struct(format);
if (s_object == NULL)
return NULL;

IMHO my struct_fastcall-5.patch is already big enough. I prefer incremental 
changes.

I have no strong opinion on cache_struct(), just a matter of taste :-) And 
others may like your change!

Feel free to write your own patch on top of mine, once mine is merged ;-)

--
Added file: http://bugs.python.org/file46434/struct_fastcall-5.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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: http://bugs.python.org/file46432/struct_fastcall-4.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

See also ascii_buffer_converter in Modules/binascii.c for example.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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 Py_CLEANUP_SUPPORTED;
}

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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() code.

Again, I would prefer to do that later. The change is already big enough :-)


> I think this should be backported to older versions.

Ok, will do once this change is accepted.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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 think this should be backported to older versions. Maybe there is an open 
issue for this.

> I would be nice to use Py_buffer format for the "buffer" argument of 
> iter_unpack(), but Argument Clinic calls PyBuffer_Release(), whereas 
> the buffer should stay alive after the function exit. Is there a way to 
> clone/copy a Py_buffer?

I agreed that the Py_buffer converter is not applicable here. Just object is 
good.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 parameter, except for iter_unpack()
* Document that unpack_from() now accepts keywords and write an unit test
* Fix Struct.__init__() error message: the method accepts also Unicode
* Document that Struct accepts str encodable to ASCII and bytes

I would be nice to use Py_buffer format for the "buffer" argument of 
iter_unpack(), but Argument Clinic calls PyBuffer_Release(), whereas the 
buffer should stay alive after the function exit. Is there a way to clone/copy 
a Py_buffer?

For the fmt/format argument: s_init() uses custom code to accept Unicode 
encodable to ASCII and bytes objects. Using Argument Clinic to check fmt type 
would require to write a converter. I would prefer to not make too many changes 
in a single patch, and I don't know how to write such converter. I suggest to 
do that later and keep the existing code. It seems like all functions getting a 
format ends in s_init() to check the format argument.

--
Added file: http://bugs.python.org/file46431/struct_fastcall-3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor added the comment:

I reformatted docstrings, and wrote a summary line.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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: http://bugs.python.org/file46422/struct_fastcall-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Why not just convert the struct module to Argument Clinic?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

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 given bytes.

Return an iterator yielding tuples unpacked from the given bytes
source according to the format string, like a repeated invocation of
unpack_from().  Requires that the bytes length be a multiple of the
format struct size.

--
components: Argument Clinic
files: struct_fastcall.patch
keywords: patch
messages: 285663
nosy: haypo, inada.naoki, larry, serhiy.storchaka
priority: normal
severity: normal
status: open
title: Modify the _struct module to use FASTCALL and Argument Clinic
type: performance
versions: Python 3.7
Added file: http://bugs.python.org/file46319/struct_fastcall.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com