Remove dead code with big performance impact#15825
Closed
dimven-adsk wants to merge 1 commit intoDynamoDS:masterfrom
Closed
Remove dead code with big performance impact#15825dimven-adsk wants to merge 1 commit intoDynamoDS:masterfrom
dimven-adsk wants to merge 1 commit intoDynamoDS:masterfrom
Conversation
dimven-adsk
changed the title
pinzart90
reviewed
Feb 13, 2025
| // Find the dependency object directly under the mouse | ||
| // cursor, then see if it represents a port. If it does, | ||
| // then determine its type, we would like to show the | ||
| // "ArcRemoving" cursor when the mouse is over an out port. |
Contributor
There was a problem hiding this comment.
looks like the purpose of this code was to : we would like to show the "ArcRemoving" cursor when the mouse is over an out port.
Also @dimven-adsk mentioned this feature was broken with this PR #13966, when the NodeView nodeHoveringStateBorder was introduced.
Should we try to restore the UI behavior or kill it for good and gain performance ? Exactly how much faster is it without this code @dimven-adsk ? That may be hard to quantify, at the very least it looks like without this code it frees up the processor a bit.
FYI @mjkkirschner @QilongTang
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
There's a piece of code that gets evaluated every time you move the mouse and currently does effectively nothing. It does nothing because the NodeView's template was modified and the end condition is never met. The mouse will never hover over a port, because the ports are always covered by the hover indicator border (**transparent backgrounds are effectively solid for hit tests):
The intended logic per the comments was to detect when the mouse is over a connected out port and display an "arc_remove.cur" cursor. Per the code, it seems the logic changed at some point to instead display an "arc_select.cur" cursor when over an input port. Either way, none of this happens and as mentioned above this code now is completely useless. Yet we still pay the cost during graph navigation, which can easily add up over time and eat away resources from elsewhere. After navigating a large graph for just a few minutes, the total CPU time can easily grow to 5-10% of the total:
I expect that if you keep working with the graph continuously for a long time, the overall percentage will craw up even higher than that.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of