Skip to content

Comments

Enhance memory data layer#3589

Closed
junshi15 wants to merge 1 commit intoBVLC:masterfrom
junshi15:MemoryDataLayer
Closed

Enhance memory data layer#3589
junshi15 wants to merge 1 commit intoBVLC:masterfrom
junshi15:MemoryDataLayer

Conversation

@junshi15
Copy link

This PR enhances memory data layer with:

forward_gpu() is added to avoid unneeded data copy
parameter "share_in_parallel" for controlling memory sharing or not (default: true)
parameter "source" for specifying the location of data source, e.g. hdfs URI

This PR introduce an optional parameter for LayerParameter:

"source_class" ... Name of custom class for parsing input data source

@junshi15 junshi15 force-pushed the MemoryDataLayer branch 2 times, most recently from 57da235 to 796d996 Compare January 27, 2016 13:34
optional LossParameter loss_param = 101;

// Specify name of source class
optional string source_class = 143;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a field in MemoryDataParameter, not LayerParameter if I understand this patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Also, this parameter (along with source) needs some more explanation, as right now they seem unused in Caffe. Why are we adding these?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is motivated by making Caffe work on Spark (via CaffeOnSpark). source_class was used by CaffeOnSpark to decide which particular Scala class to use for handling a given data source. See CaffeOnSpark code.

Our original idea is to support custom sources for data layers beyond MemoryDataLayer. For example, to support NLP use cases, we have introduced a VWDataLayer. That's why we propose an optional field at LayerParameter. By having source_class at LayerParemeter, we will enable CaffeOnSpark users to introduce customized parser for different data layers.

FYI, VWDataParameter is defined as below:
message VWDataParameter {
optional string source = 1;
optional uint32 batch_size = 2;
optional bool shuffle = 3 [default = false];
message TopLayer{
optional string name = 1;
optional string type = 2;
optional uint32 channels = 3;
optional uint32 height = 4;
optional uint32 width = 5;
optional string vw_namespace = 6;
}
repeated TopLayer top = 4;
optional bool sparse = 5 [default = false];
optional bool memory_input = 6[default = false];
}

@longjon
Copy link
Contributor

longjon commented Mar 1, 2016

Comments as noted; also, the logic here does not seem quite right.

In this patch we step through device memory in GPU mode, and host memory in CPU mode. But what if we give this layer a host pointer in GPU mode? (Or (should this even be allowed?) a device pointer in CPU mode?) It looks like this patch will break things, even though (at least the former case) works fine as-is (and is a pretty common case!).

So it seem like the action of Forward should really depend on the pointer type, not the mode.

(And given that this is not totally straightforward, it would be nice to have a test to check these cases.)

@junshi15
Copy link
Author

junshi15 commented Mar 4, 2016

Regarding the last comment, we assume memory layer gets a host pointer in the CPU mode (hence Forward_cpu) and a device pointer in the GPU mode (hence Forward_gpu). Is this a good assumption? Maybe we should add a few assertions to check that.

@longjon
Copy link
Contributor

longjon commented Mar 5, 2016

Regarding the last comment, we assume memory layer gets a host pointer in the CPU mode (hence Forward_cpu) and a device pointer in the GPU mode (hence Forward_gpu). Is this a good assumption?

No. As noted above, this assumption is violated in practice. The way I've most commonly used MemoryDataLayer is with a host pointer (the only supported option, right now) in GPU mode. That's desirable in the common case where some data already exists in host memory, and we want to run a GPU computation on it without bothering to copy all of it to device (perhaps because it won't fit, which is why we're batching through it with this layer in the first place). I don't think it's reasonable to break that use case, and I don't see any reason why the layer can't do the right thing for either mode and either pointer type.

@junshi15
Copy link
Author

junshi15 commented Mar 7, 2016

Can we assume the following:

  1. host memory is always allocated by CaffeMallocHost(), not by an arbitrary malloc().
  2. in CPU mode, this layer does not take a device pointer, i.e. Forward_cpu() won't handle device pointer.
    If those are o.k., then we can use cudaPointerGetAttributes() to detect the pointer type in the GPU mode.

@shelhamer
Copy link
Member

shelhamer commented Apr 14, 2017

Closing according to #5528. Thanks for the effort to improve this troubled layer all the same.

@shelhamer shelhamer closed this Apr 14, 2017
Copy link

@Borracho27 Borracho27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- Ojggrj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants