Reduce blocking operations in eventloop thread

[itamarst]

This is likely only a subset, but it's a start.

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/4068, will probably file follow-up for round 2.

[coveralls]

coverage: 94.627%. first build when pulling 20cfe70 on 4068-reduce-cpu-in-eventloop-thread into a08a622 on master.

[exarkun]

Thanks. It looks like many of these implementation changes should improve responsiveness during bulk operations. Maybe some of them will even improve throughput for concurrent bulk operations? I didn't try running the benchmarks and comparing against master. Did you try that? Is there a responsiveness benchmark?

Overall the new global state in the implementation is a bit bothersome. I can think of extensive refactorings that would let us avoid that but I'm not sure how realistic those are. Maybe we could try to brainstorm together and see if we can come up with something feasible that avoids it?

[exarkun]

As I understand the code, the Python ZFEC bindings don't release the GIL (which is a good thing, since until very recently the C library initialization was not thread-safe - and now requires an explicit single-threaded initialization step). Were you able to observe any throughput increase with these encode/decode changes?

[itamarst]

I mostly just put stuff in threads that the blocking-detector code flagged. I am happy to go and make ZFEC release the GIL as a relevant follow-up.

[exarkun]

Shame about that salt + block, that's presumably a lot of time spent allocating and copying and freeing in the main thread. Possibly a spot for future improvement?

[itamarst]

Copying memory is actually quite fast typically, but maybe.

[itamarst]

Here is my thought on reactor as state: it shouldn't be an attribute or parameter, it should be accessible via a function get_reactor() that internally retrieves it from contextvar.ContextVar(). And this should really be in Twisted itself...

[itamarst]

I just looked and asyncio does something similar, a thread local. ContextVar is a bit nicer in that you can override it for the current context in a stack-based way, which is useful for tests, but same basic idea.

[meejah]

"In general" trio seems to have better-thought-out async ideas (vs. asyncio) -- although I don't know what it does about the reactor.

The asyncio things I have looked at unfortunately have problems similar to many Twisted internal (and third-party) libraries when dealing with the global / implicit reactor ("thread-local context var" still seems global-adjacent to me? but maybe I don't understand ContextVar enough)

[itamarst]

1. Decided to punt on reactor, just leave API as is.
2. For testing flag, the worry with swapping out synchronous version is that you don't immediately await the value, so synchronous test doesn't catch bad parallelism. Switching the defer_to_thread() API to be a coroutine makes this less likely, you get warning if you don't await and it's not a thing you pass around, unlike Deferred.

[exarkun]

Thanks. Looks good to me.