Skip to content
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.

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

Use in-memory notifications for workers #3247

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbranson
Copy link

Changes the interprocess notifications for workers to use an in-memory multiprocessing.Value instead of changing a filesystem timestamp, which requires interaction with the filesystem and can face contention issues or I/O related bottlenecks. The changed code doesn't require any platform-specific logic because it doesn't deal directly with any platform-related idiosyncrasies. multiprocessing.Value uses shared memory under the hood in most cases so it is very efficient.

I ran a load test using "hey" against a gunicorn with a hello world app wrapped in strace -c to measure the syscall impact.

top 5 strace summary before:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 26.44    2.092495          20    100000           sendto
 14.22    1.125266          20     53774           close
 13.79    1.091552           6    160582           utimensat
 11.78    0.931931           9    100001           getsockname
 11.76    0.930716           8    105291     55291 accept4

top 5 strace summary after:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 25.83    2.058187          20    100000           sendto
 17.56    1.399223          20     67596         1 pselect6
 14.53    1.157935           9    117570     67570 accept4
 14.25    1.135567          22     50619           close
 10.89    0.867693           8    100001           getsockname

os.close(fd)
raise
def __init__(self):
self._val = multiprocessing.Value('d', lock=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if (in the O_TMPFILE and in the shm case) converting to floating point and back is needlessly weird.. We are reading the clock as a nanosecond-precision unsigned integer, are we not?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

By not using the _ns variant we ask for it. No bug, just.. why ask for binary64 (@d) to store uint64 (=Q)?

Copy link
Author

Choose a reason for hiding this comment

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

'd' is a double floating point encoding, not binary64. I'm not sure what you mean by asking for it.

@pajod
Copy link
Contributor

pajod commented Jul 18, 2024

That is an implicit ctypes dependency again, right?

@rbranson
Copy link
Author

That is an implicit ctypes dependency again, right?

multiprocessing uses ctypes internally to expose shared memory as a Python object. It doesn't create any dependencies beyond the Python stdlib and any platform differences are handled within.

@benoitc benoitc self-assigned this Jul 23, 2024
@benoitc
Copy link
Owner

benoitc commented Jul 23, 2024

The idea is good b ut I think there is a better way than using a shared value . We could instead use an atomic integer there. I will suggest an alternative asap. Let keep this PR open anyway yo compare the reult and safety.

@rbranson
Copy link
Author

The idea is good b ut I think there is a better way than using a shared value . We could instead use an atomic integer there. I will suggest an alternative asap. Let keep this PR open anyway yo compare the reult and safety.

FWIW, I thought about using a sequence number but this was a more straightforward change, and I couldn't come up with a substantial enough reason it was better. Fewer calls to get the clock?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants