Skip to content

Commit 96da149

Browse files
committed
Optimize GIL usage in PyBuffer_read()
Restructure PyBuffer_read() to eliminate unnecessary GIL roundtrips. Previously, the code released/reacquired GIL multiple times when reading all data with size=-1. Now all blocking waits and data copying happen in a single GIL-released block. - Single Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS pair - All conditional waiting inside GIL-released block - Data copying while GIL released (mutex still held) - GIL only reacquired for creating Python bytes object
1 parent 9e651ee commit 96da149

File tree

1 file changed

+50
-48
lines changed

1 file changed

+50
-48
lines changed

c_src/py_buffer.c

Lines changed: 50 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,9 @@ static void PyBuffer_dealloc(PyBufferObject *self) {
281281
*
282282
* If size is -1, read all available data (until EOF).
283283
* Returns bytes object. Returns empty bytes at EOF.
284+
*
285+
* GIL Optimization: All blocking waits and data copying happen in a single
286+
* GIL-released block to avoid unnecessary GIL roundtrips.
284287
*/
285288
static PyObject *PyBuffer_read(PyBufferObject *self, PyObject *args) {
286289
Py_ssize_t size = -1;
@@ -293,73 +296,72 @@ static PyObject *PyBuffer_read(PyBufferObject *self, PyObject *args) {
293296

294297
py_buffer_resource_t *buf = self->resource;
295298

296-
/* Release GIL during blocking wait */
299+
/* Local variables for results - set inside GIL-released block */
300+
unsigned char *local_copy = NULL;
301+
size_t to_read = 0;
302+
int closed_empty = 0;
303+
int oom_error = 0;
304+
305+
/* Release GIL for all blocking waits and data copying */
297306
Py_BEGIN_ALLOW_THREADS
298307
pthread_mutex_lock(&buf->mutex);
299308

300-
/* Wait for data if buffer is empty */
301-
while (buf->read_pos >= buf->write_pos && !buf->eof && !buf->closed) {
302-
pthread_cond_wait(&buf->data_ready, &buf->mutex);
303-
}
304-
305-
Py_END_ALLOW_THREADS
306-
307-
/* Check if buffer was closed during wait */
308-
if (buf->closed && buf->read_pos >= buf->write_pos) {
309-
pthread_mutex_unlock(&buf->mutex);
310-
return PyBytes_FromStringAndSize("", 0);
311-
}
312-
313-
/* Calculate how much to read */
314-
size_t available = buf->write_pos - buf->read_pos;
315-
size_t to_read;
316-
317309
if (size < 0) {
318-
/* Read all available (or wait for EOF if not all data yet) */
319-
if (!buf->eof && buf->content_length > 0 &&
320-
buf->write_pos < (size_t)buf->content_length) {
321-
/* Wait for all data - release mutex before reacquiring GIL to avoid deadlock */
322-
pthread_mutex_unlock(&buf->mutex);
323-
324-
Py_BEGIN_ALLOW_THREADS
325-
pthread_mutex_lock(&buf->mutex);
310+
/* Read all: wait for all content or EOF */
311+
if (buf->content_length > 0) {
312+
/* Known content length - wait for complete data */
326313
while (buf->write_pos < (size_t)buf->content_length &&
327314
!buf->eof && !buf->closed) {
328315
pthread_cond_wait(&buf->data_ready, &buf->mutex);
329316
}
330-
Py_END_ALLOW_THREADS
331-
332-
available = buf->write_pos - buf->read_pos;
317+
} else {
318+
/* Unknown length - wait for any data or EOF */
319+
while (buf->read_pos >= buf->write_pos && !buf->eof && !buf->closed) {
320+
pthread_cond_wait(&buf->data_ready, &buf->mutex);
321+
}
333322
}
334-
to_read = available;
335323
} else {
336-
to_read = (available < (size_t)size) ? available : (size_t)size;
324+
/* Read specific amount: wait for any data */
325+
while (buf->read_pos >= buf->write_pos && !buf->eof && !buf->closed) {
326+
pthread_cond_wait(&buf->data_ready, &buf->mutex);
327+
}
337328
}
338329

339-
/* Copy data while holding mutex, then release before creating PyBytes */
340-
unsigned char *local_copy = NULL;
341-
if (to_read > 0) {
342-
local_copy = (unsigned char *)malloc(to_read);
343-
if (local_copy == NULL) {
344-
pthread_mutex_unlock(&buf->mutex);
345-
PyErr_NoMemory();
346-
return NULL;
330+
/* Calculate result while holding mutex */
331+
if (buf->closed && buf->read_pos >= buf->write_pos) {
332+
closed_empty = 1;
333+
} else {
334+
size_t available = buf->write_pos - buf->read_pos;
335+
to_read = (size < 0) ? available :
336+
((available < (size_t)size) ? available : (size_t)size);
337+
338+
/* Copy data while holding mutex */
339+
if (to_read > 0) {
340+
local_copy = (unsigned char *)malloc(to_read);
341+
if (local_copy == NULL) {
342+
oom_error = 1;
343+
} else {
344+
memcpy(local_copy, buf->data + buf->read_pos, to_read);
345+
buf->read_pos += to_read;
346+
}
347347
}
348-
memcpy(local_copy, buf->data + buf->read_pos, to_read);
349-
buf->read_pos += to_read;
350348
}
351349

352350
pthread_mutex_unlock(&buf->mutex);
351+
Py_END_ALLOW_THREADS
353352

354-
/* Create PyBytes without holding mutex */
355-
PyObject *result;
356-
if (to_read > 0) {
357-
result = PyBytes_FromStringAndSize((char *)local_copy, to_read);
358-
free(local_copy);
359-
} else {
360-
result = PyBytes_FromStringAndSize("", 0);
353+
/* Create Python objects with GIL held */
354+
if (oom_error) {
355+
PyErr_NoMemory();
356+
return NULL;
357+
}
358+
359+
if (closed_empty || to_read == 0) {
360+
return PyBytes_FromStringAndSize("", 0);
361361
}
362362

363+
PyObject *result = PyBytes_FromStringAndSize((char *)local_copy, to_read);
364+
free(local_copy);
363365
return result;
364366
}
365367

0 commit comments

Comments
 (0)