Skip to content

Commit

Permalink
Make ipcfabric have a single definition
Browse files Browse the repository at this point in the history
Summary:
There is an ODR one definition rule violation that was causing a crash on sigrid
https://fb.workplace.com/groups/560979627394613/posts/2909061125919773/?comment_id=2909752389183980&reply_comment_id=2910102119149007
Sigrid includes both kineto and ipcfabric via dynolog, and on kineto the class is fused in internal. Also adding Logger.h caused a mess.

Tries to resolve this issue
Note ENABLE_IPC_FABRIC define is only used in open source kineto (and PyTorch) but not in internal fbcode

Differential Revision: D56577485
  • Loading branch information
briancoutinho authored and facebook-github-bot committed Apr 26, 2024
1 parent a5cf5ab commit 3581362
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 39 deletions.
1 change: 0 additions & 1 deletion dynolog/src/ipcfabric/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# Copyright (c) Meta Platforms, Inc. and affiliates.

add_library (dynolog_ipcfabric_lib INTERFACE)
target_compile_options(dynolog_ipcfabric_lib INTERFACE "-DENABLE_IPC_FABRIC")
43 changes: 5 additions & 38 deletions dynolog/src/ipcfabric/FabricManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,13 @@
#include "dynolog/src/ipcfabric/Utils.h"

// If building inside Kineto, use its logger, otherwise use glog
#if defined(KINETO_NAMESPACE) && defined(ENABLE_IPC_FABRIC)
// We need to include the Logger header here for LOG() macros.
#if defined USE_GOOGLE_LOG
// We need to include the Logger header before here for LOG() macros.
// However this can alias with other files that include this and
// also use glog. TODO(T131440833). Thus, the user should also set
#include "Logger.h" // @manual
// add to namespace to get logger
using namespace libkineto;

#else // KINETO_NAMESPACE && ENABLE_IPC_FABRIC
// also use glog. TODO(T131440833).
// Whoever includes this needs to also include Logger.h for use in kineto
#include <glog/logging.h>
#endif // KINETO_NAMESPACE && ENABLE_IPC_FABRIC
#endif // USE_GOOGLE_LOG

namespace dynolog::ipcfabric {

Expand Down Expand Up @@ -90,7 +86,6 @@ struct Message {
std::string src;
};

#if !defined(KINETO_NAMESPACE) || defined(ENABLE_IPC_FABRIC)
class FabricManager {
public:
FabricManager(const FabricManager&) = delete;
Expand Down Expand Up @@ -222,32 +217,4 @@ class FabricManager {
std::mutex dequeLock_;
};

#else

// Adds an empty implementation so compilation works.
class FabricManager {
public:
FabricManager(const FabricManager&) = delete;
FabricManager& operator=(const FabricManager&) = delete;

static std::unique_ptr<FabricManager> factory(
std::string endpoint_name = "") {
return nullptr;
}

bool sync_send(
const Message& msg,
const std::string& dest_name,
int num_retries = 10,
int sleep_time_us = 10000) {
return false;
}

std::unique_ptr<Message> poll_recv(int max_retries, int sleep_time_us) {
return nullptr;
}
};

#endif // !KINETO_NAMESPACE || ENABLE_IPC_FABRIC

} // namespace dynolog::ipcfabric
4 changes: 4 additions & 0 deletions dynolog/src/tracing/IPCMonitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
#pragma once

#include <memory>

// Use glog for FabricManager.h
#define USE_GOOGLE_LOG

#include "dynolog/src/ipcfabric/FabricManager.h"

namespace dynolog {
Expand Down
3 changes: 3 additions & 0 deletions dynolog/tests/ipcfabric/IPCFabricTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
// This source code is licensed under the MIT license found in the
// LICENSE file in the root directory of this source tree.

// Use glog for FabricManager.h
#define USE_GOOGLE_LOG

#include <gtest/gtest.h>
#include <linux/perf_event.h>
#include <sys/ioctl.h>
Expand Down
1 change: 1 addition & 0 deletions dynolog/tests/ipcfabric/IPCSender.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// This source code is licensed under the MIT license found in the
// LICENSE file in the root directory of this source tree.

#define USE_GOOGLE_LOG
#include "dynolog/src/ipcfabric/FabricManager.h"

int main() {
Expand Down

0 comments on commit 3581362

Please sign in to comment.