Worker: make Rust Worker the canonical Worker object#1376
Worker: make Rust Worker the canonical Worker object#1376sporksmith merged 2 commits intoshadow:devfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1376 +/- ##
==========================================
+ Coverage 54.20% 54.26% +0.06%
==========================================
Files 137 137
Lines 20565 20557 -8
Branches 5187 5181 -6
==========================================
+ Hits 11148 11156 +8
+ Misses 6493 6481 -12
+ Partials 2924 2920 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| void* _worker_run(void* voidWorkerThreadInfo) { | ||
| // Take contents of `info` and free it. | ||
| WorkerPool* workerPool = NULL; | ||
| int threadID = -1; |
There was a problem hiding this comment.
I think it would be good to have some comments in here about thread safety (and how each thread must only write to data in element threadID for the members of workerPool and not any other parts of workerPool).
There was a problem hiding this comment.
Done - added the comments to the struct definition itself and referenced them from here. It's a bit hairy now, but of course will be forced to be made more rigorous when WorkerPool itself is moved to Rust :)
src/main/core/worker.c
Outdated
| // Destroy the thread-local Worker. | ||
| worker_freeForThisThread(); |
There was a problem hiding this comment.
My understanding is that thread-locals get dropped when a (non-main) thread exits. Do we still need to do that here?
There was a problem hiding this comment.
Done
Yeah, I had been thinking destroying it explicitly might save us in the case that the Worker destructors ended up relying on other thread-locals, which could have already been destroyed if we're in that phase. Probably better to ensure any such code is resilient to the thread-local no longer being available, though. (e.g. ShadowLogger should already handle this gracefully).
| #include "main/core/support/definitions.h" | ||
| #include "main/host/protocol.h" | ||
| #include "main/routing/packet.h" | ||
| #include "support/logger/log_level.h" |
There was a problem hiding this comment.
Should the LogInfoFlags enum be moved to a separate file so that it's not before the include statements?
There was a problem hiding this comment.
Done. Oops, yeah meant to circle back and fix that.
This is a step towards moving the object graph into Rust, so that we can sort out ownership / sharing issues. The Rust Worker object is currently just a thin wrapper around the legacy C Worker object (now CWorker). It should be straightforward to start moving more members out of the CWorker directly into the Worker, but I'd like to prioritize getting all of the Shadow objects into at least this state before converting any one object completely. * The pieces of the Worker that are accessed from outside the Worker are moved into parallel arrays in the WorkerPool. This allows the Worker itself to be stored in a thread-local without any thread-safe wrappers, for cheaper access. * I removed support for 0-Workers, since it was already planned, and it'd have further complicated the previous change.
Deprecation of support for 0-worker-threads was proposed in shadow#1251 and initially implemented in shadow#1376. This cleans up some no-longer-reachable-code. In a similar spirit (that it's not worth supporting more code paths to optimize a niche use-case), it also removes the special case of not-shuffling the hosts when there is only 1 worker.
This is a step towards moving the object graph into Rust, so that we can
sort out ownership / sharing issues.
The Rust Worker object is currently just a thin wrapper around the
legacy C Worker object (now CWorker). It should be straightforward to
start moving more members out of the CWorker directly into the Worker,
but I'd like to prioritize getting all of the Shadow objects into at
least this state before converting any one object completely.
The pieces of the Worker that are accessed from outside the Worker are
moved into parallel arrays in the WorkerPool. This allows the Worker
itself to be stored in a thread-local without any thread-safe wrappers,
for cheaper access.
I removed support for 0-Workers, since it was already planned, and
it'd have further complicated the previous change.