yahoojapan / NGT

Nearest Neighbor Search with Neighborhood Graph and Tree for High-dimensional Data
Apache License 2.0
1.24k stars 114 forks source link

return std::move(xx) #31

Closed tanliboy closed 5 years ago

tanliboy commented 5 years ago

I am new to this project. Probably this question has been discussed before.

When I pull this repository and compile. It has shown copy elision warning as below:

In file included from /Users/lit/github/NGT/lib/NGT/Command.cpp:20:
/Users/lit/github/NGT/lib/NGT/Optimizer.h: In member function 'NGT::Optimizer::MeasuredValue NGT::Optimizer::measure(std::istream&, std::istream&, NGT::Command::SearchParameter&, std::pair<float, float>, double)':
/Users/lit/github/NGT/lib/NGT/Optimizer.h:620:23: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
  620 |       return std::move(v);
      |              ~~~~~~~~~^~~
/Users/lit/github/NGT/lib/NGT/Optimizer.h:620:23: note: remove 'std::move' call
[ 14%] Building CXX object lib/NGT/CMakeFiles/ngtstatic.dir/Graph.cpp.o
[ 17%] Building CXX object lib/NGT/CMakeFiles/ngtstatic.dir/Index.cpp.o
[ 21%] Building CXX object lib/NGT/CMakeFiles/ngtstatic.dir/MmapManager.cpp.o
[ 25%] Building CXX object lib/NGT/CMakeFiles/ngtstatic.dir/Node.cpp.o
[ 28%] Building CXX object lib/NGT/CMakeFiles/ngtstatic.dir/SharedMemoryAllocator.cpp.o
[ 32%] Building CXX object lib/NGT/CMakeFiles/ngtstatic.dir/Thread.cpp.o
[ 35%] Building CXX object lib/NGT/CMakeFiles/ngtstatic.dir/Tree.cpp.o
[ 39%] Linking CXX static library libngt.a
[ 39%] Built target ngtstatic
Scanning dependencies of target ngt
[ 42%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/ArrayFile.cpp.o
[ 46%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/Capi.cpp.o
[ 50%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/Command.cpp.o
In file included from /Users/lit/github/NGT/lib/NGT/Command.cpp:20:
/Users/lit/github/NGT/lib/NGT/Optimizer.h: In member function 'NGT::Optimizer::MeasuredValue NGT::Optimizer::measure(std::istream&, std::istream&, NGT::Command::SearchParameter&, std::pair<float, float>, double)':
/Users/lit/github/NGT/lib/NGT/Optimizer.h:620:23: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
  620 |       return std::move(v);
      |              ~~~~~~~~~^~~
/Users/lit/github/NGT/lib/NGT/Optimizer.h:620:23: note: remove 'std::move' call
[ 53%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/Graph.cpp.o
[ 57%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/Index.cpp.o
[ 60%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/MmapManager.cpp.o
[ 64%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/Node.cpp.o
[ 67%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/SharedMemoryAllocator.cpp.o
[ 71%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/Thread.cpp.o
[ 75%] Building CXX object lib/NGT/CMakeFiles/ngt.dir/Tree.cpp.o
[ 78%] Linking CXX shared library libngt.dylib
[ 78%] Built target ngt
Scanning dependencies of target ngt_exe
[ 82%] Building CXX object bin/ngt/CMakeFiles/ngt_exe.dir/ngt.cpp.o
In file included from /Users/lit/github/NGT/bin/ngt/ngt.cpp:18:
/Users/lit/github/NGT/lib/NGT/Optimizer.h: In member function 'NGT::Optimizer::MeasuredValue NGT::Optimizer::measure(std::istream&, std::istream&, NGT::Command::SearchParameter&, std::pair<float, float>, double)':
/Users/lit/github/NGT/lib/NGT/Optimizer.h:620:23: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
  620 |       return std::move(v);
      |              ~~~~~~~~~^~~
/Users/lit/github/NGT/lib/NGT/Optimizer.h:620:23: note: remove 'std::move' call
[ 85%] Linking CXX executable ngt
[ 85%] Built target ngt_exe
Scanning dependencies of target search
[ 89%] Building CXX object bin/search/CMakeFiles/search.dir/search.cpp.o
[ 92%] Linking CXX executable search
[ 92%] Built target search
Scanning dependencies of target ngtq_exe
[ 96%] Building CXX object bin/ngtq/CMakeFiles/ngtq_exe.dir/ngtq.cpp.o
[100%] Linking CXX executable ngtq

It is not an important thing, but should we let the compiler to do RVO instead?

tanliboy commented 5 years ago

I encountered another error while following the instructions for testing.

$ ngt create -d 128 -o c index ./data/sift-dataset-5k.tsv
ngt: Error: Specify greater than 0 for # of your data dimension by a parameter -d.
Usage: ngt create -d dimension [-p #-of-thread] [-i index-type(t|g)] [-g graph-type(a|k|b|o|i)] [-t truncation-edge-limit] [-E edge-size] [-S edge-size-for-search] [-L edge-size-limit] [-e epsilon] [-o object-type(f|c)] [-D distance-function(1|2|a|A|h|j|c|C)] [-n #-of-inserted-objects] [-P path-adjustment-interval] [-B dynamic-edge-size-base] [-A object-alignment(t|f)] [-T build-time-limit] [-O outgoing x incoming] index(output) [data.tsv(input)]

It seems lib/NGT/Command.cpp could not pass -d as expected above. Is it a KP?

masajiro commented 5 years ago

When you use macOS, the order of the command line parameters is different from that of the READMEs. Below is the order for macOS.

ngt parameters-with-a-dash command parameters

Below is an example for macOS.

ngt -d 128 -o f create index ./data/sift-dataset-5k.tsv

In addition, since I found a bug of the distance function for the byte object type, please use the float type "-o f" instead of the byte type like the example above. Although I am going to fix it as soon as possible, if you use the byte type right now, please build NGT without avx as below.

cmake -DNGT_AVX_DISABLED=ON ..
masajiro commented 5 years ago

Since the bug above depends on your CPU, you might not have the bug.

masajiro commented 5 years ago

I released NGT V1.7.8 to deal with the warning and the bug on a specific CPU.

tanliboy commented 5 years ago

@masajiro, thank you for the quick fix and explanation!