[cpp-package] Fix multiple definition issue

[sifmelcara]

Currently, if we include MxNetCpp.h in several .cpp files, linker will complains that there are multiple definition of mxnet.cpp functions, because of one definition rule.

This commit do several modifications to obey the one definition rule:

1. Add inline keyword to functions.
2. Get rid of global variable, use static class variable instead.
3. In optimizer.hpp, I delayed the registration of sgd optimizer to the first call of OptimizerRegistry::Find.
4. In kvstore.hpp, two callback functions (updater and controller) are now KVStore's static member functions. Also, extern "C" is removed because I think it is not necessary when the function is callbacked by function pointer (However, I'm not 100% sure on this point, please correct me if I'm wrong).

cc @lx75249 , thank you

[conopt]

I'm considering changing kvstore to singleton because current implementation will break when two kvstore instances are created. Or if we could make controller a dynamic function pointer which I don't believe is possible in C++. Either way, we don't need controller and updater to be static, right?

[sifmelcara]

In order to let user create multiple KVStores, I pass *this into MXKVStoreRunServer as void *controller_handle (so we can access the KVStore in the callback function). However, I have not tested the runtime behavior of the code so I'm not sure if this is a valid approach. (It seems there have no c++ example uses KVStore).

I will test the runtime behavior and correctness of these code when I have time.

[sifmelcara]

Oops, it seem I totally misunderstood how is the "name" of KVStore being used. So it may make sense to let KVStore be singleton.

[conopt]

KVStore name is its type actually (like dist_sync and dist_async). I believe we don't need to support multiple KVStore instances..

[sifmelcara]

So, if we implement KVStore as a singleton, user should use KVStore like this

KVStore::SetType("dist_sync");
KVStore::GetInstance()->Init(key, val);
KVStore::GetInstance()->RunServer();
KVStore::GetInstance()->Push(...);

Am I right?

[conopt]

Yes

[sifmelcara]

So, the usage of KVStore now looks like this:

KVStore::SetType("local");
KVStore::Init(key, val);
NDArray retrieve = NDArray(Shape(2, 3), ctx_cpu, false);
KVStore::Pull(key, &retrieve);

I have tested the basic Push and Pull functionality, please tell me if everything looks fine, thank you very much.

[conopt]

Static class is different from singleton. For example, sometimes we need to test different kvstore implementations, and it's difficult to do it with static class. And I think static methods do not have external linkage, which is the main issue you want to solve in this pull request.

[sifmelcara]

Static class is different from singleton. For example, sometimes we need to test different kvstore implementations, and it's difficult to do it with static class.

If we implement kvstore as a singleton, isn't it also hard to test different implementations? I think I'm missing something here, I would appreciated it if you could give some example.

And I think static methods do not have external linkage,

They do have external linkage, since the static keyword do not presents when we "define" the methods.

which is the main issue you want to solve in this pull request

The main issue I want to solve is, currently, when user #include "MxNetCpp.h" in several .cpp files, the "definition" of those class methods and global variables which defined in .hpp will be repeatedly included in .o files. And since c++ only allow each function or object have "one definition", these duplicate definition will cause error when we link .o files together. The solution is to add inline keyword to allow same definition appears in different translation unit, otherwise user can never include mxnet.cpp in more than one .cpp file.

[conopt]

For example, to test another implementation, we just need to modify kv = KVStore::GetInstance() to kv = AnotherKVStore::GetInstance(), and the rest remains same.

For static methods, I'm not sure if they will create multiple definitions in serverl source files. If they don't, then there is no problem.

[sifmelcara]

I tried to define another class called AnotherKVStore, and do this in my own .cpp (which includes MxNetCpp.h)

KVStore::SetType("local");
AnotherKVStore::SetType("local");

These codes work fine.

For static methods, I'm not sure if they will create multiple definitions in serverl source files. If they don't, then there is no problem.

Although these static methods create multiple definitions in several source files, they are inline, so more than one definition in the entire program is fine.

[conopt]

In this part,

KVStore::Init(key, val);
NDArray retrieve = NDArray(Shape(2, 3), ctx_cpu, false);
KVStore::Pull(key, &retrieve);

You need to replace all KVStore to AnotherKVStore, right?

[sifmelcara]

If what you are worrying is that developer needs to modify many code to test new KVStore implementation, I think typedef can be used to solve this problem.

typedef KVStore kvs;
kvs::Init(key, val);
NDArray retrieve = NDArray(Shape(2, 3), ctx_cpu, false);
kvs::Pull(key, &retrieve);

Then we only need to change typedef KVStore kvs to typedef AnotherKVStore kvs to test new implementation.

[conopt]

@piiswrong This PR LGTM. Can you help merge it? Thank you!

[piiswrong]

@yajiedesign
http://ec2-52-25-96-65.us-west-2.compute.amazonaws.com/blue/organizations/jenkins/mxnet/detail/PR-5545/7/pipeline
Any idea why windows test is using NaiveEngine?

[yajiedesign]

@piiswrong because this bug #5598, now it can change to ThreadEngine.

[piiswrong]

Could you do the fix? Or is it already done? @yajiedesign

[yajiedesign]

@piiswrong ok fixed.and rebuild.

[yajiedesign]

@piiswrong and please update branch