Skip to content

getargs.c: redundant C-contiguity check #67565

Closed
Closed
getargs.c: redundant C-contiguity check#67565
Assignees
skrah
Labels
3.13only security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usagetopic-argument-clinic

Description

@skrah
skrah
mannequin
opened on Feb 2, 2015
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

    Activity

    1. skrah
      added
      performancePerformance or resource usage
      on Feb 2, 2015

    2. skrah commented on Feb 2, 2015

      @skrah
      skrahmannequin
      on Feb 2, 2015
      MannequinAuthor

      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.

    3. vstinner commented on Feb 2, 2015

      @vstinner
      vstinner
      on Feb 2, 2015
      Member

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

    4. serhiy-storchaka commented on Feb 2, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 2, 2015
      Member

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

    5. skrah commented on Feb 2, 2015

      @skrah
      skrahmannequin
      on Feb 2, 2015
      MannequinAuthor

      Yes, _testbuffer.ndarray can create any buffer.

    6. skrah commented on Feb 2, 2015

      @skrah
      skrahmannequin
      on Feb 2, 2015
      MannequinAuthor

      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.

    7. vstinner commented on Feb 2, 2015

      @vstinner
      vstinner
      on Feb 2, 2015
      Member

      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#.

    8. vstinner commented on Feb 2, 2015

      @vstinner
      vstinner
      on Feb 2, 2015
      Member

      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?

    9. skrah commented on Feb 2, 2015

      @skrah
      skrahmannequin
      on Feb 2, 2015
      MannequinAuthor

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

    10. skrah commented on Feb 2, 2015

      @skrah
      skrahmannequin
      on Feb 2, 2015
      MannequinAuthor

      Well, here's a patch with tests.

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

    11. vstinner commented on Feb 2, 2015

      @vstinner
      vstinner
      on Feb 2, 2015
      Member

      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.

    12. skrah commented on Feb 2, 2015

      @skrah
      skrahmannequin
      on Feb 2, 2015
      MannequinAuthor

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

    13. serhiy-storchaka commented on Feb 4, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 4, 2015
      Member

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

    14. skrah
      self-assigned this
      on Apr 2, 2015
    15. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    16. iritkatriel
      added
      3.12only security fixes
      on Sep 12, 2022
    17. serhiy-storchaka
      added
      interpreter-core(Objects, Python, Grammar, and Parser dirs)
      topic-argument-clinic
      3.13only security fixes
      and removed
      3.12only security fixes
      on Aug 29, 2023
    18. serhiy-storchaka
      added a commit that references this issue on Oct 19, 2023

      gh-67565: Add tests for C-contiguity checks (GH-110951)

      9376728
    19. furkanonder
      added a commit that references this issue on Oct 23, 2023

      pythongh-67565: Add tests for C-contiguity checks (pythonGH-110951)

      498b5e7
    20. serhiy-storchaka
      added 3 commits that reference this issue on Oct 23, 2023

      [3.11] gh-67565: Add tests for C-contiguity checks (GH-110951) (GH-11…

      f446df7

      [3.12] gh-67565: Add tests for C-contiguity checks (GH-110951) (GH-11…

      d4d947d

      gh-67565: Remove redundant C-contiguity checks (GH-105521)

      32c37fe
    21. serhiy-storchaka
      closed this as completedon Oct 23, 2023
    22. aisk
      added 2 commits that reference this issue on Feb 11, 2024

      pythongh-67565: Add tests for C-contiguity checks (pythonGH-110951)

      597f897

      pythongh-67565: Remove redundant C-contiguity checks (pythonGH-105521)

      b2505b8
    23. Glyphack
      added 2 commits that reference this issue on Sep 2, 2024

      pythongh-67565: Add tests for C-contiguity checks (pythonGH-110951)

      b9acebd

      pythongh-67565: Remove redundant C-contiguity checks (pythonGH-105521)

      64491a0
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    3.13only security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usagetopic-argument-clinic

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions