Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further simplify the data layers and extend the MemoryDataLayer #995

Merged
merged 19 commits into from
Sep 3, 2014
Merged

Further simplify the data layers and extend the MemoryDataLayer #995

merged 19 commits into from
Sep 3, 2014

Conversation

kloudkl
Copy link
Contributor

@kloudkl kloudkl commented Aug 28, 2014

Completely remove the duplicate codes of the data layers.

@GeenuX, @shelhamer, @jeffdonahue

@shelhamer
Copy link
Member

This will be a welcome simplification and extension of memory data layer.

Please rebase given the merge of your switch to boost::thread in #1000.

@bhack
Copy link
Contributor

bhack commented Aug 28, 2014

How blob memory is organized? Unroll opencv mat is needed? Accessing with "at" is slow http://docs.opencv.org/doc/tutorials/core/how_to_scan_images/how_to_scan_images.html

@kloudkl
Copy link
Contributor Author

kloudkl commented Aug 29, 2014

@bhack, I didn't dive into the detailed memory layouts of blob and mat. I'm afraid that they are not guaranteed to be always consistent to avoid addressing the individual elements. The benchmark says that the iterator is the safe method and there is not much performance difference between the addressing methods. LUT function is fast but it is very intricate to manually locate each element in the raw memory.

@kloudkl kloudkl changed the title Further simplify the data layers Further simplify the data layers and extend the MemoryDataLayer Aug 29, 2014
@bhack
Copy link
Contributor

bhack commented Aug 29, 2014

We could check if Mat [is continuous](http://docs.opencv.org/modules/core/doc/basic_structures.html#bool Mat::isContinuous%28%29 const).
If not we could clone the Mat (to let it to be continuous) if it is faster then unrolling. Is in the color case Blob memory different from Opencv in storing channels?

@kloudkl
Copy link
Contributor Author

kloudkl commented Aug 30, 2014

The features are completely finished. Please review again. Thanks!

@bhack
Copy link
Contributor

bhack commented Aug 30, 2014

@kloudkl so we always want to copy the Mat data? If the Mat is 1 channel and iscontinous we could call caffe_copy because we don't need to reshape it.

@shelhamer
Copy link
Member

#1010 added a facade around boost::thread to fix compiling on OS X. Please rebase to adopt this interface then we will review the welcome refactoring.

@kloudkl
Copy link
Contributor Author

kloudkl commented Aug 31, 2014

Rebased and then GTest failed to build on my local machine.

/usr/bin/ld: .build_debug/src/gtest/gtest-all.o: undefined reference to symbol 'pthread_key_delete@@GLIBC_2.2.5'
//lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: ld returned 1 exit status
make: *** [.build_debug/test/test_hdf5data_layer.testbin] Error 1
make: *** Waiting for unfinished jobs....
/usr/bin/ld: .build_debug/src/gtest/gtest-all.o: undefined reference to symbol 'pthread_key_delete@@GLIBC_2.2.5'
//lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: ld returned 1 exit status
make: *** [.build_debug/test/test_syncedmem.testbin] Error 1

After adding pthread back in the Makefile, both the commits 81f367d and c2abd75 passed the Travis CI building as shown here and here.

@@ -95,13 +94,8 @@ DataLayer<Dtype>::~DataLayer<Dtype>() {
}

template <typename Dtype>
void DataLayer<Dtype>::LayerSetUp(const vector<Blob<Dtype>*>& bottom,
void DataLayer<Dtype>::DataLayerSetUp(const vector<Blob<Dtype>*>& bottom,
Copy link
Member

Choose a reason for hiding this comment

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

The data layers should define their DataLayerSetup() method first to fit the convention of the other layers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean LayerSetUp? Please comment on 200e7b9.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I only meant that physically in the code layout the DataLayerSetup() should be defined higher up, above InternalThreadEntry(), since in other layers LayerSetup() is defined first e.g. in ConvolutionLayer.

@shelhamer
Copy link
Member

@kloudkl thanks for the catch in #995 (comment) -- I tested on OS X 10.9 and Ubuntu 12.04 but not 14.04. I cleaned up remaining references to pthread in Caffe but restored it in the build for the gtest depenency in 4dcb96e. Please rebase to this latest fix.

@shelhamer
Copy link
Member

Including OpenCV in this way breaks the CUDA build on OS X 10.9: https://gist.github.com/shelhamer/ce6389dd4bc785258df6. Perhaps split your OpenCV contribution to the MemoryDataLayer into a follow-up PR and focus on the refactoring and inclusion of transformations in the MemoryDataLayer here.

@shelhamer
Copy link
Member

@longjon please review the changes to the MemoryDataLayer.
@jeffdonahue please review for a second opinion on the refactoring in general.

Once the OS X incompatibility is fixed this looks good to me.

@kloudkl
Copy link
Contributor Author

kloudkl commented Aug 31, 2014

On OS X 10.9, CUDA is not compatible with OpenCV. Your error log is the same as reported by Alexander Bock (@MisanthropicBit ?) in the OpenCV issue 3831.

The first solution is to work around it by wrapping OpenCV and buliding with clang/gcc.

@kamjagin solved a similar OpenCV issue 3649 in a patch which may not have been included in the public release.

He has also fixed another PCL building issue on OS X related with CUDA. Although the details of the error are not clear, I guess they are very similar to what you met with judging from no definitions of the standard library functions.

Following the examples of his method and the Caffe Makefile, the CMakeLists.txt in the root directory is enhanced in ea3f833.

+    if(APPLE)
+      SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libstdc++")
+    endif(APPLE)
    ifneq ($(findstring 10.9, $(shell sw_vers -productVersion)),)
        CXXFLAGS += -stdlib=libstdc++
        LINKFLAGS += -stdlib=libstdc++
    endif

@kloudkl
Copy link
Contributor Author

kloudkl commented Aug 31, 2014

Is it necessary to test the Clang compiler in the Travis CI?

@kamjagin
Copy link

kamjagin commented Sep 1, 2014

Unfortunately, as of 6.5 CUDA still doesn't support libc++ on mac (even though I've been promised by NVIDIA that they are working on it for a coming release :) ), so all things compiled with CUDA need to be compiled with the old standard library libstdc++ (and CUDA_HOST_COMPILER needs to be defined as /usr/bin/clang). The problem is of course that using libstdc++ limits the use of c++11 features severely.

I principle I think libc++ and libstdc++ could be linked into the same program (the nifty libc++ developers have declared everything in a separate namespace and then "use" it in the main namespace so to the linker there is no collision between the vector (etc.) of libc++ and libstdc++), but I would be quite scared of doing so anyways.

@shelhamer
Copy link
Member

@kloudkl your data layer improvements without OpenCV are a nice contribution on their own. For merge, please remove the OpenCV commits 07f9405, 38a77e4, and ea3f833 for inclusion in a follow-up.

I'd rather make progress than let this be blocked on the NVCC issue.

@kloudkl
Copy link
Contributor Author

kloudkl commented Sep 2, 2014

OpenCV was used in the WindowDataLayer and ReadImageToDatum without any problem. Before the reason why NVCC can't build it is found out, the plain old Datum is used in the MemoryDataLayer.

@shelhamer shelhamer merged commit a08f111 into BVLC:dev Sep 3, 2014
@shelhamer
Copy link
Member

@kloudkl thanks for the design improvement in a much-needed area!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants