Skip to content

WIP: Refactor out deserialize_bytelist#4077

Open
jakirkham wants to merge 5 commits into
dask:mainfrom
jakirkham:deserialize_bytelist
Open

WIP: Refactor out deserialize_bytelist#4077
jakirkham wants to merge 5 commits into
dask:mainfrom
jakirkham:deserialize_bytelist

Conversation

@jakirkham

Copy link
Copy Markdown
Member

Extracts the deserialize_bytelist function from deserialize_bytes. Under-the-hood deserialize_bytes does a minimal amount of prep work before calling into deserialize_bytelist. Thus maintaining the same code path.

This can be a useful place to hook into this part of serialization pipeline for things like in-memory compressed spilling in combination with serialize_bytelist. Also this avoids unnecessary copying in this case (unlike deserialize_bytes).

As it can be quite useful to have a way of directly working with a
`bytes`-like `list` as is provided by `serialize_bytelist`, particularly
when thinking about in-memory and/or performance-focused operations
(where additional copies can meaningfully impact both), refactor out a
`deserialize_bytelist` from `deserialize_bytes`.
Ensure `deserialize_bytelist` is available from `distributed.protocol`.
Make sure we are also able to handle collections of `bytes`-like objects
in `deserialize_bytes`. This is useful if we may be deserializing
collections of objects in-memory in some cases and deserializing data
from disk in other cases.
def deserialize_bytes(b):
try:
b = memoryview(b)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm sorry, I think I've asked this before, can you remind me when you think we would hit a TypeError?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If the object is not bytes-like, this will raise. We end up performing this coercion in unpack_frames later. So this merely performs this coercion a bit sooner.

The non-exception case is for things like spilling to disk (analogous to deserialize_bytes(serialize_bytes(obj))) and the exception case is for things like compressed in-memory spilling (analogous to deserialize_bytes(serialize_bytelist(obj))). IOW both cases can rely on using deserialize_bytes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@quasiben quasiben left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left one small question otherwise looks good

@jakirkham jakirkham changed the title Refactor out deserialize_bytelist WIP: Refactor out deserialize_bytelist Aug 25, 2020
@quasiben

Copy link
Copy Markdown
Member

There are errors in test_spill_to_disk which seem related

lengths = struct.unpack_from(f"{n_frames}{fmt}", b, fmt_size)
E struct.error: total struct size too long

@jakirkham

Copy link
Copy Markdown
Member Author

Yeah getting this right is a bit fiddly based on how the code works. I'll give it another look. Should add this is why I marked this as WIP for the moment.

Base automatically changed from master to main March 8, 2021 19:04
@jakirkham jakirkham requested a review from fjetter as a code owner January 23, 2024 10:57
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.

2 participants