Hey @trentj , thanks for the extensive feedback, much appreciated
In your case it looks like your desire to put a bound on the type itself led you to use an improper PhantomData<T>
, at least in StorageView
This was the oly way I found to solve the problem of trying o use a mutable reference multiple times in a loop. E.g.:
let js = JobSystem::new()?;
let mut v:u32 = 0;
for _ in 100 {
let x = &mut v;
let handle = js.create(|| {
x += 10;
})?;
...
}
If I don't add the PhatomData<T>
, the above code will compile just fine. I'm open to sugguestions on how to improve this.
However, in the case of StorageView<T>
that looks wrong because you're using it as a handle to a buffer ( JobStorage
) that actually does only hold one specific T
at a time. ... On that topic, why is StorageViewTrait
a thing? Seems like you could write those functions directly on StorageView
itself. Maybe I'm missing something.
The JobStorage
can hold any type T. I don't know ahead of time which type this is. I could have use a Box<dyn FnMut>()
, but my main focus with this code is to provide a 0 allocation job system after setup. The JobStorageView<T>
allows me to do the right thing via Type Erasure. In the end you could argue that I'm just reinventing the wheel, but I couldn't find any other way to store the type data, since you can't calculate a size for dyn FnMut()
.
The JobStorageView<T>
will generate 2 static functions which are unique to the type T which I then store in the JobStorage
. One of those functions do_drop
is used to drop the data when the job finishes or the job pool is destroyed.
his is unsafe because the output lifetime is not bound to the input lifetime, so you can call as_trait()
two times in a row and get aliased mutable references ... created / satisfied?
While what you said is true, I can guarantee that this is only happens once and in one place, therefore making it safe. As you said, extra documentation wouldn't hurt to illustrate this point.
Again, maybe it's not exposed to the world, but I can't easily confirm that because the way you use unsafe
isn't accurately flagging all the places where special soundness constraints need to be upheld
Are you suggesting I mark these things as unsafe to better get my point across/make it more obvious that things should be handled with care?
TLS_THREAD_DATA_INDEX
could easily be a Cell
instead of RefCell
Noted
JobThread::new doesn't seem like it should exist. You only use it to populate a Vec
and then overwrite the contents immediately. You can replace...
Noted.
Similarly, with JobSystem::new
, you start by creating the JobSystem
with dummy values, a
Noted
ThreadDataWrapper
and ThreadDataAccessor
don't, in my opinion, seem to make access to ThreadData
...
This was the only way I could find to have multiple write access for the thread queues. Each Job thread needs to able to steal jobs from another thread's queue. I looked at how Rust's mutex worked under the hood and I mimicked the pattern. RefCell
wasn't an option since it's multiple threads will request mutable borrows.
The design is still safe, but is not something that you can easily express in Rust.
You've got a lot of &mut
s for a lock-free queue. Atomics
Even though the atomics don't require mut references, I still need to mutate the underlying queue. I don't see any way around this.
What happens when there are multiple JobSystem
s? The JobHandle
...
That's a valid point. Do you have any suggestions on how to best achieve this branding? Other than this issue, the only other reason for concern would be to have multiple job systems created on the same thread. I shall address this. Thanks for pointing this out.
You can use Box<[T]>
instead of Vec<T>
when the buffer won't shrink or grow, and similarly Arc<[T]>
instead of
Ah, nice. Thanks for the tip.