-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adds explicit C API to DYAD #118
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
struct dyad_ctx; | ||
typedef struct dyad_ctx dyad_ctx_t; | ||
|
||
__attribute__ ((__visibility__ ("default"))) int dyad_c_init (dyad_ctx_t** ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was reading somewhere that the visibility attribute differs between GCC, Clang, and cray compilers. I think we should have a macro for this so that it's easier to switch to in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that. I would use the macros we have in dyad_rc.h
if I had known. What I can do is make dyad_rc.h
a public header file and use the macro here.
|
||
__attribute__ ((__visibility__ ("default"))) int dyad_c_init (dyad_ctx_t** ctx); | ||
|
||
__attribute__ ((__visibility__ ("default"))) int dyad_c_open (dyad_ctx_t* ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming it as c_open makes me think, do we need to support other semantics like creat
, openat
as semantics.
Also if the flag says O_DIRECTORY then it is not suppose to consume right? I feel we need to make it more specific.
Not use open or close. But directly dyad_produce and consume for C_APIs for DYAD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At that point, we're essentially just exposing core to the users. In fact, that's what we could do. This can be a high level C API. Essentially, it would just be a version of the wrapper that you can link into your program instead of preloading. Then, core could serve as our low level C API, providing the produce/consume interface you suggested @hariharan-devarajan.
@hariharan-devarajan @JaeseungYeom what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also let me avoid using the awkward dyad_c_...
names for these functions. The reason I chose this naming was specifically to avoid symbol conflicts between this API and core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At that point, we're essentially just exposing core to the users. In fact, that's what we could do. This can be a high level C API. Essentially, it would just be a version of the wrapper that you can link into your program instead of preloading. Then, core could serve as our low level C API, providing the produce/consume interface you suggested @hariharan-devarajan.
@hariharan-devarajan @JaeseungYeom what do you think?
I agree that we can simply expose what is already there. I think eventually we want to rename wrapper to intercepter.
@@ -6,79 +6,84 @@ | |||
#error "no config" | |||
#endif | |||
|
|||
#ifdef DYAD_PROFILER_DLIO_PROFILER // DLIO_PROFILER | |||
#ifdef DYAD_PROFILER_DLIO_PROFILER // DLIO_PROFILER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do the formatting part of different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have .clang-format
to handle formatting, although I've seen plenty of cases where it's not consistent. Not sure why at this point.
DYAD_RC_OK = 0, // Operation worked correctly | ||
DYAD_RC_SYSFAIL = -1, // Some sys call or C standard | ||
// library call failed | ||
DYAD_RC_OK = 0, // Operation worked correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do the formatting part of different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I will run formatter once all the PRs are merged.
#define DYAD_C_REGION_END(name) DLIO_PROFILER_C_REGION_END((name)) | ||
#define DYAD_C_REGION_UPDATE_INT(name) DLIO_PROFILER_C_REGION_UPDATE_INT((name), (key), (value)) | ||
#define DYAD_C_REGION_UPDATE_STR(name) DLIO_PROFILER_C_REGION_UPDATE_STR((name), (key), (value)) | ||
#define DYAD_C_FUNCTION_START() DYAD_NOOP_MACRO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing the leading space? It increases readability.
Do we need to add items into .gitignore as well? |
This PR adds an explicit C API to DYAD as a linkable library, similar to the more explicit C++ and Python libraries. This API is extremely similar to the wrapper API, and it can really be thought of as providing the same functionality as the wrapper without all the
LD_PRELOAD
stuff.