[PROF-9088] Consider timestamps in comm and fork events #366
Conversation
r1viollet
force-pushed
the
r1viollet/fix_comm
branch
from
5f598d0 to
cbf0135
Compare
To prevent clearing mmap data when it is not useful, we consider timestamps of comm and fork events
r1viollet
force-pushed
the
r1viollet/fix_comm
branch
from
cbf0135 to
cf03668
Compare
Benchmark results for collatzParameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. See unchanged results
|
Benchmark results for BadBoggleSolver_runParameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. See unchanged results
|
Ensure caches are not cleared when PID maps are not cleared
src/unwind.cc
Outdated
| if (!(us->dso_hdr.pid_free(pid, timestamp))) { | ||
| LG_DBG("(PID Free)%d -> avoid free of mappings (%ld)", pid, | ||
| timestamp.time_since_epoch().count()); | ||
| } else { |
There was a problem hiding this comment.
if the mappings are good. Does it really mean that the underlying dwfl modules are OK ?
Today we do not really tie the lifetime of the mappings to the lifetimes of the unwinding caches.
So I am not sure about this change. I think it requires more careful thinking.
@nsavoire fyi
| // Clear everything and populate at next error or with coming samples | ||
| DDRES_CHECK_FWD(worker_pid_free(ctx, frk->pid)); | ||
| DDRES_CHECK_FWD(worker_pid_free(ctx, frk->pid, timestamp)); | ||
| ctx.worker_ctx.us->dso_hdr.pid_fork(frk->pid, frk->ppid); |
There was a problem hiding this comment.
It would be better to only call pid_fork when pid mappings have actually been freed.
| if (us->_dwfl_wrapper && us->_dwfl_wrapper->_inconsistent) { | ||
| // Loaded modules were inconsistent, assume we should flush everything. | ||
| LG_WRN("(Inconsistent DWFL/DSOs)%d - Free associated objects", us->pid); | ||
| DDRES_CHECK_FWD(worker_pid_free(ctx, us->pid)); |
There was a problem hiding this comment.
It weird to have to call PerfClock::now() here, perhaps having two versions of worker_pid_free (with and without timestamp) or having PerfClock::time_point::max() as default value for timestamp in worker_pid_free would be more natural.
src/unwind.cc
Outdated
| if (!(us->dso_hdr.pid_free(pid, timestamp))) { | ||
| LG_DBG("(PID Free)%d -> avoid free of mappings (%ld)", pid, | ||
| timestamp.time_since_epoch().count()); | ||
| } else { |
Communicate the result of the unwind free to caller
r1viollet
changed the title
2 participants
What does this PR do?
To prevent clearing mmap data when it is not useful, we consider timestamps of comm and fork events.
Motivation
We encountered an issue where we gather mmap data just prior to a comm event. The comm event clears the data.
Then we are unable to compute the offset from an elf file.
Additional Notes
NA
How to test the change?
I added a unit test.
I will check with user if this fixes the issue he encountered or if we need to go further.