Add profiling mode to GUI

[s9ferech]

Pull Request Description

This adds a profiling mode with running time indicators to the graph editor, following the description of issue #1069. This PR includes just the visual part. Retrieval of the profiling information is handled in Issue #1068.

The profiling mode can be entered and exited through ctrl+p. (Holding ctrl causes the node labels to appear temporarily.)

(Outdated recording)

The appearance has an edge case when the node gets smaller than the running time label (Outdated recording):

Important Notes

Checklist

Please include the following checklist in your PR:

• The CHANGELOG.md was updated with the changes introduced in this PR.
• The documentation has been updated if necessary.
• All code conforms to the Rust style guide.
• All code has automatic tests where possible.
• All code has been profiled where possible.
• All code has been manually tested in the IDE.
• All code has been manually tested in the "debug/interface" scene.
• All code has been manually tested by the PR owner against our test scenarios.
• All code has been manually tested by at least one reviewer against our test scenarios.

[s9ferech]

I assume this should not be included in the CHANGELOG before we finish the epic, right?

[s9ferech]

The profiling mode only grays out edges currently. Should anything else be included?

[s9ferech]

The minimum and maximum running times are currently recomputed on every change, to keep things simple. If this should be too costly then I would suggest to use an existing priority queue implementation rather than the algorithm in the issue description. That should likely provide some simplicity and best performance.

[wdanilo]

1. Regarding not including it in README - why? Every task when merged should be included in the readme if it implements something that can be described from the user perspective.
2. I'm not checking the code yet, as the visual part is not implemented according to the description. The description states that the background of nodes should get the color according to its performance, there is no description of some additional borders around the nodes. When nodes get colored background, everything else should be grayed out (everything that has colors).
3. I replied to comments on the issue description, please see them as well.
4. Is one of the requirements described by Sylwia also implemented here? (the ability to profile selected nodes). If so, how can we enable it? Is there a shortcut for it?
5. Is there an icon implementation? The task states "an icon displayed in the top-right corner of the app" - the description used the word "shortcut or icon", but that was incorrect - there should be shortcut and icon - we've been talking about it during the estimation meeting.
6. Regarding the comment:

The minimum and maximum running times are currently recomputed on every change, to keep things simple. If this should be too costly then I would suggest using an existing priority queue implementation rather than the algorithm in the issue description. That should likely provide some simplicity and the best performance.

I do not understand it tbh. The algorithm described in the issue is not recomputing minimum and maximum running times on every change - and the algorithm was part of the requirements. If you want to change the requirements, let's talk about it before implementation, please. Also, we care a lot about performance here - we've been talking about it during planning / estimation meeting - I told then that If there is any more efficient algorithm, let's talk about it before implementing the task. Could you describe the algorithm that uses the priority queue step by step? If it has better performance, it should be implemented instead. However, again, we should have been talking about it before implementing this task, not after, as it would mean throwing away some part of the work - am I correct here?

[wdanilo]

It's not implemented accroding to the spec, see my comment above please.

[s9ferech]

1. Regarding not including it in README - why? Every task when merged should be included in the readme if it implements something that can be described from the user perspective.

The only thing that currently happens from the user's perspective is that ctrl+P. Switches the colors off. The profiling mode can only be used when the whole epic is finished. I would suggest to update the CHANGELOG then.

2. I'm not checking the code yet, as the visual part is not implemented according to the description. The description states that the background of nodes should get the color according to its performance, there is no description of some additional borders around the nodes. When nodes get colored background, everything else should be grayed out (everything that has colors).

To make sure that I understand it right, by background you mean the area inside the node that is behind the text and currently white?

4. Is one of the requirements described by Sylwia also implemented here? (the ability to profile selected nodes). If so, how can we enable it? Is there a shortcut for it?

I created another issue for that, as you requested. It is not implemented yet.

5. Is there an icon implementation? The task states "an icon displayed in the top-right corner of the app" - the description used the word "shortcut or icon", but that was incorrect - there should be shortcut and icon - we've been talking about it during the estimation meeting.

It is not there yet. I have a question about the design before I implement this. The icon should probably be used as indicator to let users know when they are in profiling mode. We could try to do this through color or present it as a switch between modes. Maybe we will also add more modes later. What is your opinion on this?

6. Regarding the comment:

The minimum and maximum running times are currently recomputed on every change, to keep things simple. If this should be too costly then I would suggest using an existing priority queue implementation rather than the algorithm in the issue description. That should likely provide some simplicity and the best performance.

I do not understand it tbh. The algorithm described in the issue is not recomputing minimum and maximum running times on every change - and the algorithm was part of the requirements. If you want to change the requirements, let's talk about it before implementation, please. Also, we care a lot about performance here - we've been talking about it during planning / estimation meeting - I told then that If there is any more efficient algorithm, let's talk about it before implementing the task. Could you describe the algorithm that uses the priority queue step by step? If it has better performance, it should be implemented instead. However, again, we should have been talking about it before implementing this task, not after, as it would mean throwing away some part of the work - am I correct here?

I implemented it this way, because it was almost no additional effort and makes it a bit easier to discuss the algorithm. There are slightly different ways how the priority queues could be used. What Adam suggested, would be to use a BiBTreeMap that maps node IDs to running times. Then set and remove will just keep this map updated and we can always get the min and max through the right_values iterator. I would likely make the updates faster that we need when we remove a node or reset its running time. But it would also make the other cases a little bit slower. It is probably not worth to discuss the different algorithms in detail. Let me just know which one you want and I will implement it. The one in the task description is also reasonable.

[s9ferech]

I just implemented that syntax highlighting for types will be disabled in profiling mode and the performance during the transition is incredibly bad, roughly 5-10 FPS in the interface demo scene. @farmaazon @wdanilo Does one of you have an idea why that could be?

[wdanilo]

I just implemented that syntax highlighting for types will be disabled in profiling mode and the performance during the transition is incredibly bad, roughly 5-10 FPS in the interface demo scene. @farmaazon @wdanilo Does one of you have an idea why that could be?

1. Are you using an optimized build?
2. If you have such problems, please always record the profiling in chrome and attach it here, otherwise it would be just guessing.

[s9ferech]

@MichaelMauderer, would you have time to test this PR to make sure that it works in all edge cases?

[wdanilo]

Felix, please ping me (and mark me as re-requested for the review) when this will be ready for final review.

[wdanilo]

I don't see this checked:

Please make sure that someone tested that before the merge :)

[farmaazon]

I spotted one regression during testing: I cannot change the project name.

[s9ferech]

@farmaazon

I spotted one regression during testing: I cannot change the project name.

I cannot reproduce this problem. Can you describe the exact steps?

[farmaazon]

@s9ferech

@farmaazon

I spotted one regression during testing: I cannot change the project name.

I cannot reproduce this problem. Can you describe the exact steps?

I first, did all the steps of scenario https://docs.google.com/spreadsheets/u/1/d/1RatJDM_f9_3bvYhl3Bpq2d8SyKgtVdrV1RkGxPU17c8/edit#gid=0 to the point where the project name should be changed. I was unable to edit it, neither by ctrl+click nor by double click.

Google Docs

Scenarios

Basic

Action to be done,Result,Release,Status,Why does not work?
DONE: 24/28 (86%)
Double click Enso executable.
,Unnamed project opens with two connected nodes 0 . up_to 100 . to_vector . map noise and second sort with open scatterplot visualizations. Connections should have colors. Open vi...

[s9ferech]

I first, did all the steps of scenario https://docs.google.com/spreadsheets/u/1/d/1RatJDM_f9_3bvYhl3Bpq2d8SyKgtVdrV1RkGxPU17c8/edit#gid=0 to the point where the project name should be changed. I was unable to edit it, neither by ctrl+click nor by double click.

I am still not able to reproduce this. It might be helpful if you find a minimal sequence of actions that causes this.

[farmaazon]

I first, did all the steps of scenario https://docs.google.com/spreadsheets/u/1/d/1RatJDM_f9_3bvYhl3Bpq2d8SyKgtVdrV1RkGxPU17c8/edit#gid=0 to the point where the project name should be changed. I was unable to edit it, neither by ctrl+click nor by double click.

I am still not able to reproduce this. It might be helpful if you find a minimal sequence of actions that causes this.

Well, actually just opening project and trying to rename it shows the problem.

If it is not reproducible on your side, then I will look into it tomorrow.

Google Docs

Scenarios

Basic

Action to be done,Result,Release,Status,Why does not work?
DONE: 24/28 (86%)
Double click Enso executable.
,Unnamed project opens with two connected nodes 0 . up_to 100 . to_vector . map noise and second sort with open scatterplot visualizations. Connections should have colors. Open vi...

[mwu-tow]

@s9ferech @farmaazon I have the same issue on develop, however only if I build with gnu-flavored rustc toolchain. Also, it seems to happen only in dist command.

[s9ferech]

That is very interesting. For a long time we had a problem that the error stripes appeared in front of nodes on some systems, but not on others, right? Could that be related? I think the breadcrumbs contain some invisible shapes as mouse targets. Maybe the objects are in the wrong order when you run it and the mouse events do not arrive at the right one. Does that sound possible?

[mwu-tow]

@s9ferech Interesting theory, might be possible. TBH recently I haven't seen the error stripes issue but if such ordering issues can appear and disappear on its own, then there might be actually something wrong in that area. Relying on some unspecified behavior or something else that makes objects in different order depending on build environment.

[farmaazon]

The bug will be fixed once #1620 will be closed.