Closed OoJJBoO closed 8 months ago
Hi, many thanks for finding the issue. It is probably there for a long time already because the perfmon module is finalized in likwid_markerClose
since the beginning.
Your approach seems reasonable with the only downside that the file will be written twice, once in the new likwid_writeMarkerFile
and again in the final LIKWID_MARKER_CLOSE
.
The biggest issue is that it requires changes in likwid.h
. My approach is to change the main likwid.h
only in major releases and I just released 5.3
. Maybe I can make an exception because it just adds a function and does not change the behavior of the already published functions.
While reading over C-internalMarkerAPI
I saw some typos. Since you already work on that file, would it be possible for you to fix the typos?
Sure, I'll look into it.
I just updated the comments in C-internalMarkerAPI.c
, feel free to let me know if I missed something @TomTheBear. I also adjusted the description regarding reading marker files since parts of it looked to be outdated.
I merged your PR #584 and started with similar functions for the GPU-related Marker APIs #586 . I think we can close the issue.
Yes, thank you!
Describe the bug
As of commit 1a50c17, the perfmon functions provided for working with marker files (
perfmon_readMarkerFile
,perfmon_getNumberOfRegions
,perfmon_getEventsOfRegion
, and so on) are effectively broken.This is because they, as far as I understand, need
likwid_markerClose
to be called for the marker file to actually be written first, which, since the mentioned commit, also finalizes the performance monitoring facilities. This, in turn, causesperfmon_readMarkerFile
to always fail with the errorwhen trying to read the created marker file afterward because perfmon was already finalized and the needed data does not exist anymore.
To Reproduce
To reproduce, simply run the provided
C-internalMarkerAPI
example or try callingperfmon_readMarkerFile
afterlikwid_markerClose
has written a marker file.Possible Fix / Workaround
Since it looks to be complicated to remove the
perfmon_finalize
call fromlikwid_markerClose
, I think another possible way to address this issue would be to add a new function that only writes the marker file without erasing the performance monitoring data. That way, users would be able to write the marker file manually and subsequently read results using the existing API functions.I already implemented this approach in my local fork and it seems to be working so far after some quick experiments. The
likwid_writeMarkerFile
function I added is a modified copy of the relevant code inlikwid_markerClose
with perfmon calls replacing theLikwidResults
object created byhashTable_finalize
.Output of
C-internalMarkerAPI
using the current repository state (commit b611b8e):Output of the same example using my
likwid_writeMarkerFile
function instead oflikwid_markerClose
:The results seem to be equivalent to the ones I got when simply removing the perfmon finalization from
likwid_markerClose
, so my implementation probably does what it's supposed to :)Do you think this could be an acceptable solution @TomTheBear?
Edit: To make comparing a bit easier, I created a pull reques with the described additions: #584