Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dead code with big performance impact #15825

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dimven-adsk
Copy link
Contributor

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):

image

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:

RwARVKvimh

EtPF3kAVp4

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

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release 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

@dimven-adsk dimven-adsk changed the title Remove dead code with big performance impact: Remove dead code with big performance impact 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.
Copy link
Contributor

@pinzart90 pinzart90 Feb 13, 2025

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants