classification
Title: getargs.c: redundant C-contiguity check
Type: performance Stage:
Components: Versions: Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: skrah Nosy List: larry, martin.panter, serhiy.storchaka, skrah, vstinner
Priority: normal Keywords: patch

Created on 2015-02-02 12:10 by skrah, last changed 2015-04-02 10:57 by skrah.

Files
File name Uploaded Description Edit
issue23376.diff skrah, 2015-02-02 12:17 review
issue23376-2.diff skrah, 2015-02-02 21:41 review
Messages (12)
msg235244 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-02 12:17
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.
msg235247 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-02 12:45
Do you have unit test with non contiguous buffers? If not, it would help to have such buffer in _testcapi.
msg235248 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-02 12:52
I think memoryview(bytearray)[::2] provides non contiguous buffers. But I'm not sure this is tested.
msg235250 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-02 12:53
Yes, _testbuffer.ndarray can create any buffer.
msg235252 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-02 12:59
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.
msg235253 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-02 12:59
> 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#.
msg235258 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-02 13:39
> 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?
msg235276 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-02 17:29
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).
msg235289 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-02 21:41
Well, here's a patch with tests.

Victor, I think you added the contiguity test in 9d49b744078c. Do you
remember why?
msg235290 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-02 21:44
> 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.
msg235291 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-02 22:03
Ah yes, it seems to originate from #3139.
msg235395 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-04 19:16
See also contiguity tests in Modules/binascii.c and Modules/_ssl.c,
History
Date User Action Args
2015-04-02 10:57:37skrahsetassignee: skrah
2015-04-01 22:46:39martin.pantersetnosy: + martin.panter
2015-02-04 19:16:40serhiy.storchakasetmessages: + msg235395
2015-02-02 22:03:39skrahsetmessages: + msg235291
2015-02-02 21:44:59vstinnersetmessages: + msg235290
2015-02-02 21:41:45skrahsetfiles: + issue23376-2.diff

messages: + msg235289
2015-02-02 17:29:19skrahsetmessages: + msg235276
2015-02-02 13:39:17vstinnersetmessages: + msg235258
2015-02-02 12:59:37vstinnersetmessages: + msg235253
2015-02-02 12:59:10skrahsetmessages: + msg235252
2015-02-02 12:53:44skrahsetmessages: + msg235250
2015-02-02 12:52:00serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg235248
2015-02-02 12:45:18vstinnersetnosy: + vstinner
messages: + msg235247
2015-02-02 12:17:17skrahsetfiles: + issue23376.diff

nosy: + larry
messages: + msg235244

keywords: + patch
2015-02-02 12:10:22skrahcreate