Skip to content

gh-150942: Speed up csv.reader row building #150995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
omkar-334 wants to merge 4 commits into
base: main
Choose a base branch
from
+5 −3
Open

gh-150942: Speed up csv.reader row building #150995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7945e61
gh-150942: Speed up csv.reader row building
omkar-334 Jun 6, 2026
1c00c00
Merge branch 'main' into gh-150942-csv-appendtakeref
omkar-334 Jun 6, 2026
ed09dac
add patch contributor name
omkar-334 Jun 6, 2026
b29bdae
Merge branch 'main' into gh-150942-csv-appendtakeref
omkar-334 Jun 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2026-06-05-12-00-00.gh-issue-150942.Hkdh87.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Speed up :func:`csv.reader` by appending parsed fields to the output row
without an extra reference-count round-trip (using the internal
reference-stealing list append helper). Patch by Omkar Kabde.
5 changes: 2 additions & 3 deletions Modules/_csv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module instead.
#endif

#include "Python.h"
#include "pycore_list.h" // _PyList_AppendTakeRef()
#include "pycore_pyatomic_ft_wrappers.h"

#include <stddef.h> // offsetof()
Expand Down Expand Up @@ -685,11 +686,9 @@ parse_save_field(ReaderObj *self)
}
self->field_len = 0;
}
if (PyList_Append(self->fields, field) < 0) {
Py_DECREF(field);
if (_PyList_AppendTakeRef((PyListObject *)self->fields, field) < 0) {

@eendebakpt eendebakpt Jun 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list used here is self->fields. Can we guarantee that it is not mutated by concurrent threads?

@omkar-334 omkar-334 Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when parse_save_field is running, self->fields is only reachable through self, and self is locked by `Reader_iternext's critical section. i dont think any other thread can mutate the list.

static PyObject *
Reader_iternext(PyObject *op)
{
    PyObject *result;
    Py_BEGIN_CRITICAL_SECTION(op);
    result = Reader_iternext_lock_held(op);
    Py_END_CRITICAL_SECTION();
    return result;
}

@omkar-334 omkar-334 Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think?

@omkar-334 omkar-334 Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @eendebakpt , just a nudge

@eendebakpt eendebakpt Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object is locked and self->fields is not accessible via other paths so this is indeed safe.

If redesigning I would make probably make fields (and maybe some other fields) a local variable inside Reader_iternext_lock_held (only minor thing to take care of: the fields currently acts as a guard for re-entrant calls). This is out of scope for the PR though.

I would drop the new entry (or at least omit the implementation details).

@omkar-334 This needs to be reviewed by a core dev, might it might take some time.

return -1;
}
Py_DECREF(field);
return 0;
}

Expand Down
Loading