[mars2] mars multi-instance. #1128

panchangpeng · 2023-05-25T02:28:27Z

No description provided.

tencent-adm · 2023-05-25T02:28:42Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

garryyan · 2023-05-25T11:12:05Z

mars/app/app_manager.h

+#include "mars/comm/thread/thread.h"
+#include "mars/comm/time_utils.h"
+
+using namespace mars::comm;


头文件中不要使用全局的 using namespace，会污染命名空间

garryyan · 2023-05-25T11:17:20Z

mars/boot/context.cc

+
+#define S_SCOPED_LOCK() std::unique_lock<std::recursive_mutex> s_lock(s_mutex_)
+#define S_SCOPED_UNLOCK() s_lock.unlock()
+static int sg_context_instance_counter = 0;


这个变量好像没啥用？

garryyan · 2023-05-25T11:20:37Z

mars/boot/context.h

+        std::string clazz_name = typeid(T).name();
+        // when context delete, then manager map will be null.
+        if (!manager_map_.empty()) {
+            return (T*)manager_map_[clazz_name];


这里一定会存在相应的 manager 吗

garryyan · 2023-05-25T11:27:31Z

mars/sdt/sdt_manager.cc

+
+void SdtManager::OnDestroy() {
+    delete sdt_core_;
+    if (callback_) {


这里的判断应该是没必要的

garryyan · 2023-05-25T11:27:58Z

mars/sdt/sdt_manager.h

+#include "mars/boot/context.h"
+#include "sdt.h"
+
+using namespace mars::boot;


头文件中不应该有 using namespace

garryyan · 2023-05-25T11:51:16Z

mars/sdt/src/checkimpl/httpquery.cc

@@ -184,7 +184,7 @@ int SendHttpQuery(const std::string& _url, int& _status_code, std::string& _errm
        }

        // fix SplitHttpHeadAndBody crash
-        if (0 == recv_autobuf.Length()) {
+        if ( 0 == recv_autobuf.Length()) {


格式化的问题

garryyan · 2023-05-25T11:52:05Z

mars/sdt/src/sdt_core.h

@@ -31,6 +31,9 @@
 #include "mars/sdt/constants.h"
 #include "mars/sdt/sdt.h"
 #include "mars/sdt/netchecker_profile.h"
+#include "mars/boot/context.h"
+
+using namespace mars::boot;


garryyan · 2023-05-25T12:44:10Z

mars/stn/src/net_source.cc

    xinfo_function();
+    weak_network_logic_ = new WeakNetworkLogic();


放在构造函数的初始化列表中？

garryyan · 2023-05-25T12:48:40Z

mars/stn/src/shortlink_task_manager.cc

@@ -313,6 +319,9 @@ void ShortLinkTaskManager::__RunOnStartTask() {

        first->task.shortlink_host_list = hosts;

+        if (hosts.empty()) {


这里为什么会存在 empty 的情况？之前不存在吗

garryyan · 2023-05-25T12:50:51Z

mars/stn/stn_manager.h

+#include "mars/boot/context.h"
+#include "mars/stn/stn.h"
+
+using namespace mars::boot;


antterminator · 2023-05-29T12:52:07Z

mars/boot/context.h

+    }
+
+    void RemoveManagerWithName(std::string name, BaseManager* mgr) {
+        std::string clazz_name = mgr->GetName();


mgr 参数是否必要？看起来只用 name 就足够了

这个是给Android的接口，因为name对应的是Java对象，mgr取的是c++对象。

如果name能唯一标识一个Java/C++对象，删除C++对象直接这样就行了吧？

std::string clazz_name = java_clazz_name_c_clazz_name_map_[name];
manager_map_.erase(clazz_name);

antterminator · 2023-05-29T13:10:34Z

mars/boot/jni/context_wrapper.cc

+ public:
+    static void JniCreateContext(JNIEnv* env, jobject instance, jstring context_id) {
+        auto context_cpp = boot::Context::CreateContext(j2c_cast(context_id));
+        auto contextWrapper = new jnicat::JniObjectWrapper<Context>(context_cpp);


原先 JniObjectWrapper 的实现很冗余，其实没必要 new JniObjectWrapper 对象出来，本质是调用 setHandle，getHandle，可以考虑简化。

antterminator · 2023-05-30T02:23:42Z

mars/boot/context.h

+    }
+
+    void* GetManagerByName(std::string name) {
+        std::string clazz_name = java_clazz_name_c_clazz_name_map_[name];


这里先find一下，不存在返回nullptr比较合理，否则两个map中会存在很多无用数据

antterminator · 2023-05-30T02:28:17Z

mars/boot/context.cc

+        }
+    } else {
+        auto context = new Context();
+        auto tmp_context_id = std::to_string(reinterpret_cast<uint64_t>(context));


对象地址很容易复用，直接用对象地址作为context_id并不能保证唯一性。如果context_id需要全局唯一，比如log中要能区分出来，最好拼上一个自增id

antterminator · 2023-05-30T02:39:49Z

mars/stn/stn_manager.h

+                const AutoBuffer& _extend);
+    // 底层获取task要发送的数据
+    bool Req2Buf(uint32_t taskid,
+                 void* const user_context,


无意义的const

antterminator · 2023-05-30T02:39:57Z

mars/stn/stn_manager.h

+                 AutoBuffer& outbuffer,
+                 AutoBuffer& extend,
+                 int& error_code,
+                 const int channel_select,


无意义的const

antterminator · 2023-05-30T07:35:46Z

mars/app/app_logic.cc

@@ -108,16 +122,28 @@ NO_DESTROY static Thread sg_slproxyThread;
        lock.lock();
        sg_proxyInfo.ip = ips.front();
        sg_gotProxy = true;
-
+        */
+        AppManager* app_manager = Context::CreateContext("default")->GetManager<AppManager>();


冗余代码太多了，考虑抽成一个函数，用的时候：
if (auto app_manager = GetDefaultAppManager()) {
app_manager->xxx();
}

或者直接 GetDefaultAppManager()->xxx() 也行

antterminator · 2023-05-30T12:14:52Z

mars/sdt/sdt_logic.cc

    xinfo2(TSF"sdt oncreate");
    SdtCore::Singleton::Instance();
+    */
+    SdtManager* sdt_manager = Context::CreateContext("default")->GetManager<SdtManager>();


大量冗余代码，同之前，建议抽成函数

antterminator · 2023-05-30T12:17:13Z

mars/sdt/sdt_logic.cc

+    xassert2(NULL != sdt_manager, "mars2 sdt_manager is empty.");
+    if (sdt_manager) {
+        sdt_manager->OnDestroy();
+    }
 }

 static void __initbind_baseprjevent() {

 	GetSignalOnCreate().connect(&onCreate);


多实例之后，旧的onCreate，onDestroy机制是不是可以干掉了？

[mars2] mars multi-instance.

d91a8b4

garryyan reviewed May 25, 2023

View reviewed changes

mars/stn/stn_manager.h

#include "mars/boot/context.h"

#include "mars/stn/stn.h"

using namespace mars::boot;

Copy link

Collaborator

garryyan May 25, 2023

Choose a reason for hiding this comment

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

using

antterminator reviewed May 29, 2023

View reviewed changes

antterminator reviewed May 30, 2023

View reviewed changes

mars/stn/stn_manager.h

AutoBuffer& outbuffer,

AutoBuffer& extend,

int& error_code,

const int channel_select,

Copy link

antterminator May 30, 2023

Choose a reason for hiding this comment

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

无意义的const

antterminator reviewed May 30, 2023

View reviewed changes

antterminator approved these changes May 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mars2] mars multi-instance. #1128

[mars2] mars multi-instance. #1128

panchangpeng commented May 25, 2023

tencent-adm commented May 25, 2023

garryyan May 25, 2023

garryyan May 25, 2023

garryyan May 25, 2023

garryyan May 25, 2023

garryyan May 25, 2023

garryyan May 25, 2023

panchangpeng May 25, 2023

garryyan May 25, 2023

garryyan May 25, 2023

garryyan May 25, 2023

garryyan May 25, 2023

antterminator May 29, 2023

panchangpeng May 30, 2023

antterminator May 30, 2023

antterminator May 29, 2023

antterminator May 30, 2023

antterminator May 30, 2023

antterminator May 30, 2023

antterminator May 30, 2023

antterminator May 30, 2023

antterminator May 30, 2023

antterminator May 30, 2023

antterminator May 30, 2023

		xinfo_function();
		weak_network_logic_ = new WeakNetworkLogic();

		@@ -313,6 +319,9 @@ void ShortLinkTaskManager::__RunOnStartTask() {

		first->task.shortlink_host_list = hosts;

		if (hosts.empty()) {

[mars2] mars multi-instance. #1128

Are you sure you want to change the base?

[mars2] mars multi-instance. #1128

Conversation

panchangpeng commented May 25, 2023

tencent-adm commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment