I still don't have a good feeling on what timescales where currently optimizing or whether this is a particularly performance critical section. Therefore, this comment might be irrelevant.
However, exception handling is relatively expensive and if we encounter a lot of cache misses a isin should be faster. That's ns level optimization. I could imagine the pickling is usually an order of magnitude slower and this doesn't matter at all

Any reason why protocol=4 is hard coded?

I'm also curious about this

I am curious too :)
This is taken directly from

That line comes from @mrocklin's PR ( #4019 ), which allowed connections to dynamically determine what compression and pickle protocols are supported and then use them in communication. In a few places I think Matt found it easier to simply force pickle protocol 4 than allow it to be configurable. So if this is coming from that worker code, that is the history

I think we can be a bit more generous with the cache size. currently we're at 100 (LRU maxsize) * 100_000 B (result) ~ 1MB. Considering how much stuff we're logging without taking size into account too much, I would suggest to be more generous with this upper limit since large results are the juicy cache hits.

Related: in loads_function, what if we used hash(bytes_object) as the key instead of bytes_object itself? Then we wouldn't have to hang onto references to those large bytestrings that we won't look at again.

It sounds like that was just copied and moved over from here

Perhaps we can make a new issue and revisit?

Perhaps we can make a new issue and revisit?

My plan is to remove worker.dumps_function() completely, it shouldn't be required to call it explicitly.

Ah ok in that case I don't think the protocol=4 bit above will be needed

personal preference: I would put the size of the cache into a constant s.t. the two function don't drift apart

Related: in loads_function, what if we used hash(bytes_object) as the key instead of bytes_object itself? Then we wouldn't have to hang onto references to those large bytestrings that we won't look at again.

Are set and frozenset necessary here, since they can't contain lists or dicts, even recursively within tuples?

>>> {([])}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'list'

Do we still care about whether pickle is used or not, now that we have msgpack_persist_lists?

Related: what happens if a MsgpackList gets pickled? Won't it be passed on (in a task, say) as a MsgpackList, not a plain list? Whereas msgpack_decode_default returns them as plain lists.

What would happen if we did this instead? obj.data should already be a list, so I'm wondering if the extra copy to a tuple is necessary.

@jrbourbeau I think that you might want to be aware of this change

Functions can be quite large sometimes, for example if users close over large variables out of function scope. Msgpack may not handle this well in some cases

x = np.arange(1000000000)

def f(y):
    return y + x.sum()

Obviously users shouldn't do this, but they will.

It looks like we're bypassing the list of serializers here. This allows users to get past configurations where users specifically turn off pickle.

The inplace stuff always makes me uncomfortable. Thoughts on making new header/frames dict/lists here instead?

For reference, it was these sorts of inplace operations that previously caused us to run into the msgpack tuple vs list difference. I think that avoiding them when we can is useful, unless there is a large performance boost (which I wouldn't expect here).

I am confused about when this is necessary and why it wasn't before. I'm wary of creating new systems like this if we can avoid it.

I think I understand this now that I've seen the c.submit(identity, Foo()) test below

What is the type of obj here? Is in the right test here, or is this special value in a more specific place?

I'm curious, why did we drop this test?

I'm curious, what happened here?

Hrm, you mentioned this in meeting a couple of weeks ago. I see now how this is unfortunate.

I would expect this test to now be written as

with pytest.raises(CancelledError):
    await c.submit(identity, Foo())

I wouldn't expect the other lines here to be indented. In general seeing assert statements under a raises context manager is a sign that something is unclean :)

I have changed it to make it more clear what is going on:

    # Notice, because serialization is delayed until `distributed.batched`
    # we don't get an exception immediately. The exception is raised and logged
    # when the ongoing communication between the client the scheduler encounters
    # the `Foo` class. Before <https://github.com/dask/distributed/pull/4699>
    # the serialization happened immediately in `submit()`, which would raise the
    # `MyException`.
    with captured_logger("distributed") as caplog:
        future = c.submit(identity, Foo())
        # We sleep to make sure that a `BatchedSend.interval` has passed.
        await asyncio.sleep(c.scheduler_comm.interval)
    # Check that the serialization error was logged
    assert "Failed to serialize" in caplog.getvalue()

I'm curious, what do you think of the approach? We cannot easily catch the exception because it happens as part of the ongoing communication and not in the submit() call, but at least we log the exception.

If the user has specified that they don't want to allow serialization with pickle then these should continue to fail. Probably we need to feed the list of serializers down wherever serialize is beting called. I expect that this might be awkward to do when going through msgpack machinery. Maybe there is some global that we can misuse?

These sorts of changes are probably not ok. They fundamentally change the intent of the test, which is to ensure that things like this can be disallowed.

This has been fixed

I'm also curious about this

It looks like we're bypassing the list of serializers here. This allows users to get past configurations where users specifically turn off pickle.

Since we are discussing getting rid of dumps_function, will these still be needed or will they go away as well?