Add PETSc logging infrastructure #2655
Conversation
ZoeLeibowitz
requested review from
EdCaunt,
FabioLuporini,
JDBetteridge and
mloubout
devito/ir/cgen/printer.py
Outdated
| def _print_MathFunction(self, expr): | ||
| return f"{self._ns}{self._print_DefFunction(expr)}" | ||
|
|
||
| def _print_String(self, expr): |
There was a problem hiding this comment.
I'm surprised this is needed?
and if it is, can you rather do it like at lines 400, 401 etc ?
There was a problem hiding this comment.
I've updated the String.__str__ instead, in a similar way to how Class and MacroArgument are handled
devito/operator/operator.py
Outdated
|
|
||
| def _emit_language_info(self): | ||
| if self._language == 'petsc': | ||
| return petsc_summary(self.parameters) |
There was a problem hiding this comment.
I think you want the runtime arguments here (namely args, that the caller should have passed), not the static ones (self.parameters)
There was a problem hiding this comment.
I need access to the static self.parameters to construct the PetscSummary. I'm retrieving PetscInfo objects, which are similar to the Timer object in that callers don't explicitly pass them
devito/operator/operator.py
Outdated
| return perf_summary | ||
|
|
||
| # Combine the performance and language specific summaries | ||
| return CombinedSummary(perf=perf_summary, lang=lang_summary) |
There was a problem hiding this comment.
why, instead of CombinedSummary, not just appending the information to perf_summary itself (it is a mutable data structure anyway)
I mean somewhere around here: https://github.com/devitocodes/devito/blob/main/devito/operator/profiling.py#L480
There was a problem hiding this comment.
also note that the profiler object is (optionally) available inside the IET compiler passes, so perhaps you could anticipate something in there to eventually make the handling of the PETSc profiling part more homogeneous wrt what happens in devito for "normal" (as in, non-PETSc) Operators.
Anyway, feel free to ignore this, not a priority
There was a problem hiding this comment.
I've removed CombinedSummary and instead dynamically added PetscSummary as a property to PerformanceSummary. If users specify language='petsc', they can access the language specific summary via perf_summary.petsc. What do you think? It also makes it easy to extend support for other summary types in the future?
There was a problem hiding this comment.
I though we previously discussed not just including these in the Performance summary, since they aren't performance statistics.
There was a problem hiding this comment.
I think PerformanceSummary could just be renamed as Summary?
EdCaunt
left a comment
EdCaunt
left a comment
There was a problem hiding this comment.
Looks great! A handful of tiny comments, but nothing important
ZoeLeibowitz
force-pushed
the
petsc_performance_summary
branch
from
401c4e6 to
16be19a
Compare
Add PETSc logging infrastructure. The data available to users via the
PetscSummarywill be extended in future PRs.