Skip to content

Merge frames in deserialize_bytes #3639

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

Merged
merged 9 commits into from
Jun 5, 2020
Merged

Merge frames in deserialize_bytes #3639

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
33f197f
Test byte serialization in frame splitting case
jakirkham Jun 4, 2020
33594d3
Merge frames in `deserialize_bytes`
jakirkham Jun 4, 2020
59afbde
Add fast-path when only one frame is needed
jakirkham Jun 4, 2020
9ab510f
Test serializing a collection of `bytes` objects
jakirkham Jun 4, 2020
f688b21
Avoid copying `bytes`
jakirkham Jun 4, 2020
198f81f
Only fast-path single frame case
jakirkham Jun 4, 2020
3b14b99
Drop fast-path for single frame
jakirkham Jun 5, 2020
eafbf77
Drop zero frame case
jakirkham Jun 5, 2020
63a2086
Wrap `**` with with `int`
jakirkham Jun 5, 2020
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
6 changes: 5 additions & 1 deletion distributed/protocol/serialize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@
import msgpack

from . import pickle
from ..utils import has_keyword, typename
from ..utils import has_keyword, nbytes, typename
from .compression import maybe_compress, decompress
from .utils import (
unpack_frames,
pack_frames_prelude,
frame_split_size,
merge_frames,
ensure_bytes,
msgpack_opts,
)
Expand Down Expand Up @@ -473,6 +474,8 @@ def replace_inner(x):

def serialize_bytelist(x, **kwargs):
header, frames = serialize(x, **kwargs)
if "lengths" not in header:
Copy link
Member

Choose a reason for hiding this comment

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

We (you :) ) recently added lengths to CUDA object headers. When is lengths not in header ? Is this something we should be requiring ?

Copy link
Member Author

@jakirkham jakirkham Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah still wrapping my head around this. My understanding is the header from the object has already gone through msgpack at this point so is actually a frame as well. So it may be we always need to set the lengths here.

header["lengths"] = tuple(map(nbytes, frames))
frames = sum(map(frame_split_size, frames), [])
Copy link
Member Author

@jakirkham jakirkham Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Note that the frames are already split here. This is due to constraints caused by compression (which happens below).

if frames:
compression, frames = zip(*map(maybe_compress, frames))
Expand All @@ -499,6 +502,7 @@ def deserialize_bytes(b):
else:
header = {}
frames = decompress(header, frames)
frames = merge_frames(header, frames)
return deserialize(header, frames)


Expand Down
16 changes: 13 additions & 3 deletions distributed/protocol/tests/test_serialize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,19 @@ def test_empty_loads_deep():
assert isinstance(e2[0][0][0], Empty)


def test_serialize_bytes():
for x in [1, "abc", np.arange(5), b"ab" * int(40e6)]:
b = serialize_bytes(x)
@pytest.mark.parametrize(
"kwargs", [{}, {"serializers": ["pickle"]},],
)
def test_serialize_bytes(kwargs):
for x in [
1,
"abc",
np.arange(5),
b"ab" * int(40e6),
2 ** 26 * b"ab",
(2 ** 25 * b"ab", 2 ** 25 * b"ab"),
Copy link
Member

Choose a reason for hiding this comment

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

Beware here that I've seen Python pre-compute literals like these as an unfortunate optimization, which results in large constant values in the program code. It might be wise to call a function around 2 **26 like int(2 ** 26) even though it has no semantic effect.

My bad experience on this was a long time ago, and so maybe things have improved..

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm...yeah I'm not sure. Did push some int wrappers around these though. One of these cases was already here (so we may have already been ok).

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, probably not a problem then. Thanks again for resolving the full issue quickly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course 🙂

]:
b = serialize_bytes(x, **kwargs)
assert isinstance(b, bytes)
y = deserialize_bytes(b)
assert str(x) == str(y)
Expand Down
3 changes: 0 additions & 3 deletions distributed/protocol/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,6 @@ def merge_frames(header, frames):
"""
lengths = list(header["lengths"])

if not frames:
return frames

assert sum(lengths) == sum(map(nbytes, frames))

if all(len(f) == l for f, l in zip(frames, lengths)):
Expand Down
4 changes: 3 additions & 1 deletion distributed/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,9 @@ def ensure_bytes(s):
>>> ensure_bytes(b'123')
b'123'
"""
if hasattr(s, "encode"):
if isinstance(s, bytes):
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe bytes(s) does make a copy if s is already bytes

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm...maybe I'm misremembering. In any event we seem to have similar code in Dask.

Copy link
Member

Choose a reason for hiding this comment

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

OK, well never mind - this version shouldn't hurt. I wonder why not just call the dask version? I suppose this one can work on memoryviews and bytearrays too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was thinking about that as well, but didn't want to go down a rabbit hole here. Am ok pulling this out into a separate PR so we can explore orthogonally. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I'm saying your version is fine :)

Copy link
Member Author

@jakirkham jakirkham May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW updating the Dask implementation to contain the code from Distributed in PR ( dask/dask#9050 ). Then we should be able to switch to the Dask implementation in Distributed.

Edit: Switched over to using the Dask implementation in Distributed with PR ( #6295 ).

return s
elif hasattr(s, "encode"):
return s.encode()
else:
try:
Expand Down