getargs.c: redundant C-contiguity check

[skrah]

BPO	23376
Nosy	@vstinner, @larryhastings, @skrah, @vadmium, @serhiy-storchaka
Files	issue23376.diff issue23376-2.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/skrah'
closed_at = None
created_at = <Date 2015-02-02.12:10:22.226>
labels = ['performance']
title = 'getargs.c: redundant C-contiguity check'
updated_at = <Date 2015-04-02.10:57:37.861>
user = 'https://github.com/skrah'

bugs.python.org fields:

activity = <Date 2015-04-02.10:57:37.861>
actor = 'skrah'
assignee = 'skrah'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2015-02-02.12:10:22.226>
creator = 'skrah'
dependencies = []
files = ['37976', '37987']
hgrepos = []
issue_num = 23376
keywords = ['patch']
message_count = 12.0
messages = ['235244', '235247', '235248', '235250', '235252', '235253', '235258', '235276', '235289', '235290', '235291', '235395']
nosy_count = 5.0
nosy_names = ['vstinner', 'larry', 'skrah', 'martin.panter', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue23376'
versions = ['Python 3.5']

Linked PRs

• gh-67565: remove redundant C-contiguity checks #105521
• gh-67565: Add tests for the remove redundant C-contiguity checks #110951
• [3.12] gh-67565: Add tests for C-contiguity checks (GH-110951) #111198
• [3.11] gh-67565: Add tests for C-contiguity checks (GH-110951) #111199

[skrah]

The call to PyBuffer_IsContiguous() (see patch) is redundant:

PyBUF_WRITABLE is a flag that can be added to any buffer request.
The real request here is (PyBUF_SIMPLE|PyBUF_WRITABLE), which is
equal to PyBUF_WRITABLE since PyBUF_SIMPLE==0.

PyBUF_SIMPLE implies C-contiguous with format 'B'.

Perhaps the check was added for broken buffer providers, but I
think at some point we should assume correct providers.

[vstinner]

Do you have unit test with non contiguous buffers? If not, it would help to have such buffer in _testcapi.

[serhiy-storchaka]

I think memoryview(bytearray)[::2] provides non contiguous buffers. But I'm not sure this is tested.

[skrah]

Yes, _testbuffer.ndarray can create any buffer.

[skrah]

A unit test would not be so helpful. The potential problem is that
third party extensions with broken getbufferprocs would suffer.

But at some point we have to streamline PEP-3118 code, or it
will remain inscrutable forever. Extension writers often copy
code from cpython, and I'm afraid if we don't remove redundancy,
people will think such contiguity checks are necessary.

[vstinner]

Yes, _testbuffer.ndarray can create any buffer.

Cool. So could you please add non regression tests to test_w_star() of test_getargs2?

Other formats expect a contiguous buffer: 'y*', 's*', 'z*'.
Formats which "convert" a buffer: 'y', 's#', 'z#.

[vstinner]

A unit test would not be so helpful. The potential problem is that
third party extensions with broken getbufferprocs would suffer.

I don't understand the link between third party extensions and
test_getargs2. test_getargs2 is a unit test for non-regression of
CPython. Can you please elaborate?

[skrah]

STINNER Victor wrote:

I don't understand the link between third party extensions and
test_getargs2. test_getargs2 is a unit test for non-regression of
CPython. Can you please elaborate?

A test failure needs a broken buffer provider that hands out a non-contiguous
buffer in response to a PyBUF_SIMPLE request.

The only non-contiguous buffer provider in the stdlib is memoryview.
If I break memoryview's getbufferproc ...

diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c
--- a/Objects/memoryobject.c
+++ b/Objects/memoryobject.c
@@ -1465,11 +1465,6 @@
         return -1;
     }
     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;
     }
     if (!REQ_SHAPE(flags)) {

...

test_buffer already fails. So what should I test? Perhaps ctypes has
some capabilities that I'm unaware of.

If I'm using _testbuffer.ndarray(), I'm effectively testing that the
getbufferproc of ndarray is correct (which is also tested multiple times
in test_buffer).

[skrah]

Well, here's a patch with tests.

Victor, I think you added the contiguity test in 9d49b744078c. Do you
remember why?

[vstinner]

Victor, I think you added the contiguity test in 9d49b744078c. Do you
remember why?

I don't understand the change like that. The call to PyBuffer_IsContiguous(view, 'C') was older than this changeset.

[skrah]

Ah yes, it seems to originate from bpo-3139.

[serhiy-storchaka]

See also contiguity tests in Modules/binascii.c and Modules/_ssl.c,