Open hobofan opened 8 years ago
Interface can be implemented like this (breaking change):
pub struct Tensor<'a> {
dim: Cow<'a, [usize]>,
memory: Rc<MemoryType>,
}
pub struct SharedTensor {
dim: Vec<usize>,
/// Current version and memory descriptor.
copies: LinearMap<Device, (usize, Rc<MemoryType>)>,
latest_version: usize,
}
impl SharedTensor {
pub fn new(dim: Vec<usize>) -> SharedTensor {
SharedTensor {
dim: dim,
copies: LinearMap::new(),
latest_version: 1,
}
}
pub fn read<'a>(&'a self, device: &Device) -> Result<Tensor<'a>, Error> {
// Check if there is initialized data anywhere
// Lookup memory and it's version for `device`, allocate it if it doesn't exist
// Check version, if it's old, syncronize
Ok(Tensor {dim: Cow::Borrowed(&self.dim)})
}
pub fn read_write<'a>(&'a mut self, device: &Device) -> Result<Tensor<'a>, Error> {
// Same as ::read(), but also increase mem version and latest_version
Ok(Tensor {dim: Cow::Borrowed(&self.dim)})
}
pub fn write_only<'a>(&'a mut self, device: &Device) -> Result<Tensor<'a>, Error> {
// Same as ::write(), but skip initialization check
Ok(Tensor {dim: Cow::Borrowed(&self.dim)})
}
}
SharedTensor
holds copies and their version numbers. User can request any number of immutable Tensor
s or a single mutable Tensor
(enforced by borrowck). It's possible to validate in runtime that tensor data is initialized when user requests Tensor
for reading and allow initialization to be skipped if Tensor
is requested only for writing.
It's also possible to add some restricted slicing to Tensor
.
What I don't like here:
Tensor
doesn't contain any info if it's writable or not. Someone can write into it by mistake. It's possible to create two types Tensor
and MutTensor
, latter can have methods for getting writable memory. Or there may be other ways to encode mutability in type system.Framework
in Tensor
and DeviceType
type signatures: SharedTensor::read<'a, F: Framework>(&'a self, device: &F::D) -> Tensor<'a, F>
. I have a feeling that this kind of approach taken further would remove a lot of enum wrappers, unwraps, and would allow to move all frameworks out of collenchyma and remove feature flags. That's not for free though: some things would have to be wrapped in smart pointers, since size of framework-related structures wouldn't be known at collenchyma's compile time. But I doubt that would add perceptable runtime cost for Leaf usecases.I've created a draft/prototype here. It's still work in progress, but please let me know what do you think about it!
I've updated the prototype. There is full list of features in comments in the head of the file, here is short version:
Tensor
struct that contains dimensions along with memory pointer. Tensor
can be reshaped without affecting it's parent SharedTensor
. It's also possible to implement restricted form of slicing on Tensor
.SharedTensor
to be mutable. Mutability is only required to change contents.collenchyma-cuda
, collencyma-opencl
, so there is mostly no need for feature flags. See cuda mock implementation. This has slight runtime cost: Device
/Merory
has to be wrapped in Any
and unwrapped on synchronization.What is not done:
@hobofan @MichaelHirn I'm ready to refactor collenchyma, plugins and leaf according to this proposal, but I'd like to know your opinion on it beforehand. It'd be a waste if after all is done it'd turn out that it was clear from the start that PR wouldn't be accepted. Of course during refactoring something unexpected may still show up and it'll turn out that those changes are a bad idea after all, but that's another matter.
Hey @alexandermorozov,
That is pretty huge, I looked through the code and read about the intention. Following my thoughts and ideas: (It might be, that I take concepts from your earlier comments, which are no longer in the code. I try to avoid it, though.)
Costs of decoupling frameworks and shared tensor. I am a big fan of this approach. Do you have any tests on performance on this one? Also, @hobofan, do you have a feeling for how profound this decoupling would be in terms of runtime performance? Quote:
it would be nice to include Framework in Tensor and DeviceType type signatures: SharedTensor::read<'a, F: Framework>(&'a self, device: &F::D) -> Tensor<'a, F>. I have a feeling that this kind of approach taken further would remove a lot of enum wrappers, unwraps, and would allow to move all frameworks out of collenchyma and remove feature flags. That's not for free though: some things would have to be wrapped in smart pointers, since size of framework-related structures wouldn't be known at collenchyma's compile time. But I doubt that would add perceptable runtime cost for Leaf usecases.
SharedTensor
is, differs in a fundamental way. The role of a SharedTensor
is to track the location of memory across devices for one conceptual similar piece of data. I am not a fan of giving the memory copies a way to change its shape. In my opinion, this makes it harder to reason about what's the state of a SharedTensor and adds functionality, that is not really needed. But I am open, to see what the intention for this design approach is and what benefits it holds.Conclusion:
I like your approach and your proof of concept. I just have the feeling, that the code changes might have gone a bit further than necessary. Before we move further, I would like to find a solution that gives us all the benefits while changing only what is really needed.
@MichaelHirn, hi!
I agree, scope is rather big, but that's mostly to see what is possible within this approach and what is impossible, to avoid restricting future possibilities. I don't think that every feature mentioned here should be implemented. Answering your questions:
SharedTensor
wasn't initialized and return an error if somebody tried to borrow it for reading. It's also possible to implement the same thing with array of bool flags (or BitVec
/BitSet
) that indicate if each memory contains latest data or not. Well, it looks like implementation would be simpler and faster, I should try it instead of versioning.Device
and Memory
beforehand. b) Use of Any
and downcasts. AFAIK under the hood downcast is a method call on a trait object that compares struct ids, so it should be reasonably fast, c) another decoupling-related thing: if Native in defined in collenchyma, and Cuda -- in collenchyma-cuda, then without feature flags only Cuda may implement sync in/sync out from Native, since Native doesn't know anything about Cuda. So we have to try two calls, since generic code doesn't know about possible transfer directions betweed objects with erased type: native_src.sync_out(cuda_dst) and cuda_dst.sync_in(native_src). First call will fail, second will do the work. So, one extra sync attempt. It could be fixed if Native is made aware of Cuda, but, well, it spoils decoupling a bit and introduces feature flags again.Tensors
doesn't affect it's parent SharedTensor
. That would destroy locality and will lead to quite a few unpleasant surprises for users. In current implementation Tensor
has COW dimensions data. In Leaf source there are places where during forward pass output tensors are reshaped to the form required by the next layer, on each pass. That requires SharedTensor
s to be mutable. Maybe that's why nearly every tensor is wrapped in ArcLock? If that's the case, then temporary Tensor
that can be reshaped could help.Tensor
).read_write()
, write_only()
) it's expected that it'll write something to the memory, so other locations are immediately considered outdated. If memory is borrowed with write_only()
, it's not syncronized, and caller is expected to overwrite memory completely. Failure to do so may result in use of uninitialized memory later. It's not perfect, but mostly cost-free and much better than nothing.Can you tell more about multi-device and async? Multi-device as in sync between OpenCL devices and Cuda? Or between several Cuda devices? If latter, then would they share same Cuda context? From cursory glance it looks like async transfers require pinned host memory, is this the case? That may not mix well with native ndarray types... And it looks like from performance point of view collenchyma should be able to set up several Cuda -> Hosts transfers and be able to wait on all of them for completition...
Nice, thank you so much for the clarification. Helped a lot.
I think there are some great concepts, that we should lend from and use for the 0.1 release of Collenchyma. I am very excited to move this forward with you. I will make some free time to expand my answer and suggestions on Thursday.
Wow, that looks pretty awesome!
It generally looks good to me. As already mentioned in 1. I think an enum instead of version numbers are probably faster and should be well optimizable by the compiler.
Apart from that I can only provide the bikeshedding opinon that I think MutTensor
should be named Tensor
and Tensor
should be named TensorView
. It seams that similar systems (in other languages) already follow that convention. However not a overly strong opinion on that.
@hobofan Well, if you mean enums like DeviceType
and MemoryType
, then they are quite compatible with use of version numbers / bitsets. They don't play well with decoupling... Those enums have to be defined in the base crate, so base crate needs to explicitly enumerate Cuda, OpenCl and other known backends, mostly defeating the benefits. But full decoupling is nice -- it'll make it easier to experiment with new backends, like GPU-over-Capnproto or something.
I'll begin to work on a stand-alone PR for enchanced synchronization, since it mostly has no downsides (and big upsides!). After that I'll try to benchmark/implement decoupling, it'll become clear what it costs in runtime and if it's worth it.
@alexandermorozov I meant using a enum like
enum SyncState {
Uninitalized,
Outdated,
Latest
}
though that would require setting all other copies to Outdated
every time one is updated. Not sure if thats more or less efficient than using version numbers.
@hobofan Ah, I see. Well, in practice Uninitialized
should be the same as Outdated
-- they both mean that this memory is not for reading. So there are really only 2 states and bool will do. Bools can be packed into an integer and the integer can be set/reset in one operation. u64
will limit max number of memories to 64. Looks like that should be enough for all foreseeble use cases. Does anyone really need to keep more than 64 copies? Maybe something like holding the same tensor on multiple nodes in a cluster? If implementation turns out to be restricting, it can be easily modified to use BitSet
instead of single integer in exchange for some runtime cost. Sorry I wasn't clear -- the initial proposal was to use versions, later while replying to @MichaelHirn it became clear that BitSet
is enough, I mentioned it but haven't elaborated, now it seems like u64 will be even better...
I need to push it until tomorrow, I think. I reviewed the topic re the SharedTensor
extensively now, but I need to do the write up of it tomorrow. I hope I can do it tomorrow.
@MichaelHirn No problem! I'm quite busy too, most likely I won't have enough mindspace to implement anything until weekend.
I discussed most of the topics with @hobofan as well last week and we reached a conclusion on nearly all points. The summary is, that the implementations you proposed make a lot of sense and would provide great value for the future.
Also, sorry for the delay, the last days were kind of unusual and I wanted to make sure, that I can follow the reasoning of the new SharedTensor
proposal.
SharedTensor
is usually not tracking to many Tensor
s anyways. So the overhead for setting all other Tensor
s to outdated might be neglectable. This would keep the logic super simple. I suspect, that the other solutions would require more edge case handling and might make debugging really hard. But we are open discussing this further if needed.TensorView
for the immutable interface and Tensor
for the mutable one.SharedTensor
(or the Tensor
; not sure yet). That would introduce a bit more management on the operation/functionality side, but could be abstracted behind a macro.
I think whatever we do with the SharedTensor
now, multi-device support can be applied anytime later on. Memory
as before. Actually work is already underway, but it'll take some time, including fixing breakage in plugins and Leaf. I'll post updates on progress when logical chunks will be ready. For now I'll use u64
to store bitmasks -- that's super fast, requires no extra allocations and no access indirection. The only downside is hard limit on number of tracked memories. If need to store more arises, it'll be easy to replace u64
with slower BitSet
.Layer::forward()
step. Looks like most of those Vecs will hold just one element. For most NNs cost should be insignificant, but there are possible corner cases. It'd be nice to use slices on stack instead of Vecs, but I don't know how easy it is.SharedTensor
-> Tensor
transition, and wait for completion on Tensor
-> Memory
request. But it looks like it won't solve all cases where async can help... I had a brief look with the cuda tool and it looks like GPU is idle most of the time because of gpu <-> host transfers; currently solver requires them in several places. My not so fast GTX 960 is typically loaded at 20-30% as reported by nvidia-smi and something like 14% by profiler if I remember correctly. Of course, solver should be cleaned up to require as few transfers as possible, but I wonder how much speedup can be squeezed by aggressively stuffing pipeline in advance...Here are links related PRs: autumnai/collenchyma#62 autumnai/collenchyma-blas#15 autumnai/collenchyma-nn#48 autumnai/leaf#103 autumnai/leaf-examples#17
In my experience those PRs have introduced no regressions and can be merged. Though it's better to resolve autumnai/collenchyma-nn#46 before that, so I could port those changes to my patches.
leaf-examples mnist mlp --batch-size 10
are now 16% faster (run time is down from 31.0s to 26.1s). I've not changed anything besides synchronization code, seems like there were extra syncs...
The current way of syncing a SharedTensor is a bit unintuitive and in some parts not quite complete.
Things to consider changing (loosely ordered by importance):
add_device
to one that doesn't return anError
if the Tensor is already tracking memory for that device. The current behaviour is unintuitive. This change is breaking and will require adjustments in all plugins.add_device
andsync
. This should clean up end-applications and the "managed" part of plugins.Native
without manually having to use the function from1.