Skip to content

Allow the DSL to provide a task_snprintf function and use it when displaying the DOT#409

Merged
abouteiller merged 5 commits intoICLDisco:masterfrom
devreal:task-snprintf
Aug 15, 2022
Merged

Allow the DSL to provide a task_snprintf function and use it when displaying the DOT#409
abouteiller merged 5 commits intoICLDisco:masterfrom
devreal:task-snprintf

Conversation

@devreal
Copy link
Copy Markdown
Contributor

@devreal devreal commented Aug 8, 2022

Backport from the TTG branches...

Signed-off-by: Thomas Herault herault@icl.utk.edu
Signed-off-by: Joseph Schuchart schuchart@icl.utk.edu

…playing the DOT

Signed-off-by: Thomas Herault <herault@icl.utk.edu>
Signed-off-by: Joseph Schuchart <schuchart@icl.utk.edu>
@devreal devreal requested a review from a team as a code owner August 8, 2022 21:37
Comment thread parsec/parsec_prof_grapher.c Outdated
char tmp[MAX_TASK_STRLEN], nmp[MAX_TASK_STRLEN];
char sim_date[64];
parsec_task_snprintf(tmp, MAX_TASK_STRLEN, context);
if(NULL != context->task_class->task_snprintf) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make it the default to use the task class accessor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean we should provide task_snprintf callbacks for DTD and PTG by default?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, always use task_class->task_snprintf.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there are two ways of going with this. We're using parsec_task_snprintf in many places and all of them would have to be adjusted:

  1. Use parsec_task_snprintf as the default implementation in task_class. That would require replacing all calls to parsec_task_snprintf with task->task_class->task_snprintf.
  2. Inside parsec_task_snprintf, check if task_class->task_snprintf is set and if not fallback to the current implementation. This would not require us to touch all the places that call parsec_task_snprintf.

I'm leaning to (1) although it will make this PR significantly bigger...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1) is perfectly fine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1) should be implemented now.

NB: debugging still falls back on parsec_task_snprintf(). We may want to change this to always call task_class->task_snprintf (or have a more robust version that checks if it exists for debugging purpose, as Joseph suggested). For now, only parsec_prof_grapher uses task_class->task_snprintf. I think changing everything could be another PR, but if you think we should also do that change now, speak up before merging.

@abouteiller abouteiller merged commit bd970e3 into ICLDisco:master Aug 15, 2022
therault added a commit to therault/parsec that referenced this pull request Dec 1, 2023
This feature was lost in a previous merge accident, but it
should have been enabled via PR ICLDisco#409
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants