Skip to content

Read & write all frames in one pass #4506

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

Jump to bottom
Merged
mrocklin merged 12 commits into from
Feb 17, 2021
Merged

Read & write all frames in one pass #4506

mrocklin merged 12 commits into from
Feb 17, 2021

Conversation

@jakirkham
Copy link
Member

@jakirkham jakirkham commented Feb 12, 2021 •
edited
Loading

  • Closes #xxxx
  • Tests added / passed
  • Passes black distributed / flake8 distributed

Instead of doing multiple reads with async, just allocate one big chunk of memory for all of the frames and read into it. Should cutdown on the number of passes through Tornado needed to fill these frames.

Also knowing that IOStream has an internal queue of buffers for writing, we are able to push all of the frames into that queue beforehand. Then ask Tornado to write after they are in the queue. This also cuts down on the number of passes through Tornado by simply entering the write handling code once and writing all the buffers.

@mrocklin
Copy link
Member

mrocklin commented Feb 12, 2021

Hrm, neat. :)

@jakirkham
Copy link
Member Author

jakirkham commented Feb 12, 2021

Yeah we might be able to do better with sendmsg and recvmsg. Am still wrapping my head around how we can use those

@mrocklin
Copy link
Member

mrocklin commented Feb 12, 2021

My guess is that going beyond this will have diminishing returns, at least on the scheduler side where we generally have small messages. I could easily be wrong though.

@jakirkham jakirkham force-pushed the read_all_frames branch 3 times, most recently from 1b7d376 to 7021240 Compare February 13, 2021 03:32
@jakirkham
Copy link
Member Author

jakirkham commented Feb 13, 2021 •
edited
Loading

Made a few more changes. I think this captures the same idea as what sendmsg/recvmsg would give us (except for the ability to send multiple buffers at once). By doing this we are able to go from 2 read_bytes calls down to 1 read_bytes calls. Should add this is in addition to having one big buffer receiving all frames.

@jakirkham jakirkham force-pushed the read_all_frames branch 10 times, most recently from 83f3e8b to ff4047f Compare February 16, 2021 03:29
@jakirkham
Overwrite frames variable w/joined result
a8d32f3 
Ensures that the separate `frames` are freed from memory before
proceeding to the next step of sending them out.
@jakirkham
Read all frames in one pass
2e2fe13 
Instead of doing multiple reads with `async`, just allocate one big
chunk of memory for all of the frames and read into it. Should cutdown
on the number of passes through Tornado needed to fill these frames.
@jakirkham
Transmit total frame size in initial message
aa0e1d2 
This should allow us to allocate space for the entirety of the rest of
the message including the size of each frames and all following frames.
We can then unpack all of this information once received. By doing this
we are able to cutdown on addition send/recv calls that would otherwise
occur and spend less time in Tornado's IO handling.
@jakirkham
Handle nframes with rest of message
bccd0e6
@jakirkham
Drop unneeded variable assignment
489c20b
@jakirkham
Use packing and unpacking utility functions
77971c1 
Simplifies the code in the TCP path by leveraging existing utility
functions.
@jakirkham
Fix-up variable names
e27dce0 
Make it a little easier to follow how the variables relate to each
other.
@jakirkham
Move packing/unpacking steps out of try
25e1672 
These are just binary serialization steps that are not really depended
on communication or issues that may come from sockets. So go ahead and
move them out of the `try` block.
@jakirkham
Copy link
Member Author

jakirkham commented Feb 16, 2021 •
edited
Loading

Simplified this a bit more using pack_frames_prelude to build the header for us. Then we just tack on the message size before that so the receiving end gets that first.

On the receiving end, we just get the message size first. Then use that to preallocate a buffer to hand off to Tornado to fill. The remaining unpacking is just handled by unpack_frames. Deserializing steps can then be handed off after that per usual.

@jakirkham
Group header building steps together
2e140b9 
Also group `frames` and `frames_nbytes` steps together. Finally rewrites
the code to avoid use of constant for size of `"Q"`, which should make
it invariant to changes in that size.
@jakirkham
Use struct.calcsize to get size
d12d0d9 
Should avoid issues on platforms where this may not be the exact size.
@jakirkham
Concatenate small frames beforehand
39f4f71 
To simplify the logic, just concatenate small frames before doing any
sends. This way we can use the same code path for all sends.
@jakirkham jakirkham changed the title Read all frames in one pass Read & write all frames in one pass Feb 16, 2021
@jakirkham
Copy link
Member Author

jakirkham commented Feb 16, 2021

Also figured out how to offload all frames to Tornado. So it now only uses one write call there

@jakirkham
Use trick to enqueue frames to write beforehand
3e37aa9 
Tornado has an internal queue that uses to hold frames before writing
them. To avoid needing to track and wait on various `Future`s and the
amount of data sent, we can just enqueue all of the frames we want to
send before a send even happens and then start the write. This way
Tornado already has all of the data we plan to send once it starts
working. In the meantime, we are able to carry on with other tasks while
this gets handled in the background.

https://github.com/tornadoweb/tornado/blob/6cdf82e927d962290165ba7c4cccb3e974b541c3/tornado/iostream.py#L537-L538
@jakirkham jakirkham mentioned this pull request Feb 16, 2021
Open
@jakirkham
Copy link
Member Author

jakirkham commented Feb 16, 2021 •
edited
Loading

Some profiling details in issue ( quasiben/dask-scheduler-performance#108 ). Most notably this is giving us a 7% improvement in transfer time for the shuffle benchmark ( quasiben/dask-scheduler-performance#108 (comment) )

@jakirkham
Copy link
Member Author

jakirkham commented Feb 17, 2021

Please let me know if anything else is needed here 🙂

@jakirkham jakirkham mentioned this pull request Feb 17, 2021
Open
@mrocklin mrocklin merged commit 383ea03 into dask:master Feb 17, 2021
@jakirkham jakirkham deleted the read_all_frames branch February 17, 2021 16:41
@jakirkham
Copy link
Member Author

jakirkham commented Feb 17, 2021

Thanks Matt! 😄

@mrocklin
Copy link
Member

mrocklin commented Feb 17, 2021 via email

@jakirkham jakirkham mentioned this pull request Feb 17, 2021
Closed
@fjetter fjetter mentioned this pull request Feb 23, 2021
Closed
@gjoseph92 gjoseph92 mentioned this pull request Jul 23, 2021
Closed
3 tasks
@jakirkham jakirkham mentioned this pull request Jul 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jakirkham @mrocklin