GH-126833: Dumps graphviz representation of executor graph. #126880
Conversation
There was a problem hiding this comment.
Some PEP 7 nits.
At risk of being pedantic, can the graph arrow be based on the direction of the branch taken? That is, when the branch is false it points left, and when true it goes right. This way we can tell the directionality of the branch without having to refer to the source code.
Co-authored-by: Ken Jin <[email protected]>
There is no direction to the arrow. It is up to the layout tool to decide where to put things. |
There was a problem hiding this comment.
This mostly looks okay. I'm not super familiar with graphviz, but otherwise the logic makes sense. There's just two minor comment about the return value of find_line_number(). Other than that, the other comments are essentially just suggestions for comments clarifying what's going on. As a reader, it took me a minute to sort things out, since I don't really know much graphviz, and the suggested comments would have helped.
Also, what are the options for testing this?
| int line = find_line_number(code, executor); | ||
| fprintf(out, ": %d</td></tr>\n", line); |
There was a problem hiding this comment.
Would it be worth handling the lno-not-found case?
For example:
| int line = find_line_number(code, executor); | |
| fprintf(out, ": %d</td></tr>\n", line); | |
| int line = find_line_number(code, executor); | |
| if (line > 0) { /* lno is 1-based. */ | |
| fprintf(out, ": %d", line); | |
| } | |
| fprintf(out, "</td></tr>\n", line); |
That said, what you have is probably fine. I doubt it would be confusing.
|
When you're done making the requested changes, leave the comment: |
| fprintf(out, " <tr><td port=\"i%d\" border=\"1\" >%s -- %" PRIu64 "</td></tr>\n", i, opname, inst->execution_count); | ||
| #else | ||
| fprintf(out, " <tr><td port=\"i%d\" border=\"1\" >%s</td></tr>\n", i, opname); |
There was a problem hiding this comment.
Can you please add oparg and operand0 information as well? I've found it incredibly useful to debug polymorphism.
There was a problem hiding this comment.
Not in this PR.
Happy to do so in another PR, or review yours 🙂
Dumps out the executor graph in graphviz format.
I'm not sure where to the put the function, for now I've named it
sys._dump_tracelets()It definitely shouldn't be part of the public API.