Introduce Log-levels in DART (2) #316
Conversation
…mutual exclusion between threads and checks log-levels
dart-impl/base/src/logging.c
Outdated
| dart_global_unit_t unit_id; | ||
| dart_myid(&unit_id); | ||
| // avoid inter-thread log interference | ||
| dart_mutex_lock(&logmutex); |
There was a problem hiding this comment.
Acquiring a mutex for every log message has significant overhead, but more important, it also leads to implicit synchronization. This is only needed for multi-threaded logging, right? In this case, I suggest to wrap the dart_mutex_* calls in #ifdef DART_ENABLE_THREADSUPPORT.
There was a problem hiding this comment.
This is actually handled in dart_mutex_*, which resolves to an empty static inline call if DART_ENABLE_THREADSUPPORT is not defined.
There was a problem hiding this comment.
That's what I assumed, but dart_mutex_* is part of the DART interface, the "empty" implementation is just in DART-MPI. So any other implementation of dart_mutex_*, e.g. in DART-SHMEM or DART-GASPI would depend on this assumption, too. (Stichwort Schichtenbruch).
There was a problem hiding this comment.
dart_mutex_* is actually part of the dart_base interface, not the public DART interface. Thus, it should be safe to use in other modules (shmem, tasking, ...)
There was a problem hiding this comment.
Well, yes, but that adds to my point. That is: we need the #ifdef guards in both places, the logging functions and the implementation of dart_mutex_*.
There was a problem hiding this comment.
No it doesn't. Both logging.h and mutex.h are part of the dart-base interface. logging.c is compiled as part of dart-base. The modules that use the DART_LOG* macros will never even see the dart_mutex_* calls in dart__logging__message. So if there is no support for for threads, logging.c will be compiled with an empty version to dart_mutex_* and anything linking against dart_base will call that compiled function. Of course, if you decide to build dart_base and dart_shmem with different threading settings other things are likely to break...
There was a problem hiding this comment.
Ok, I see. But shouldn't dart_mutex_* be renamed to dart_base_mutex_* then? Not sure how I did this with the locality stuff in dart/base, though.
There was a problem hiding this comment.
Yes, I used dart__base__locality* for the common functions in dart/base. We should do the same for dart_mutex*.
There was a problem hiding this comment.
It's now dart__base__mutex* (although I am not a big fan of the double underscore namespace thingy...)
There was a problem hiding this comment.
Me neither, but it's all we got. Life is hard without colons.
dart-impl/base/src/logging.c
Outdated
| "[ %*d %.5s ] [ %*d ] %-*s:%-*d %.3s DART: %s\n", | ||
| DASH__DART_LOGGING__UNIT__WIDTH, unit_id.id, | ||
| loglevel_names[level], | ||
| DASH__DART_LOGGING__PROC__WIDTH, pid, |
There was a problem hiding this comment.
These macros could be renamed to DART_LOGGING__PROC_WIDTH etc., I used the prefix DASH__ back then but it's pretty stupid to write DASH__DART... in hindsight. Spelled out, it's DASH DASH Runtime ;)
There was a problem hiding this comment.
Done. Also, there is no need to have it in the header file anymore.
…o feat-dart-loglevel
…o feat-dart-loglevel
Giving this another shot (refer to #247 for previous discussion). This PR adds the following on top of it:
DART_LOG*functions to a single function call (printf-style argument checking is preserved). This yields some significant savings in binary size (151kB vs 240kB forlibdart-mpi.awith debug output enabled)Related to #109