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

Coalesce graph editor view invalidations #6786

Merged
merged 3 commits into from
May 29, 2023

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented May 19, 2023

Pull Request Description

Only invalidate the graph editor view at most once per frame. On develop, this saves about 70ms (2%). Testing a recent backend without #6755 as a stress-test, this saves about 5s (45%). This reflects better scalability to large numbers of SuggestionUpdate messages.

Fixes #6630.

Important Notes

  • Also fix intermittent profiling failures occurring since the introduction of microtasks.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@kazcw kazcw self-assigned this May 19, 2023
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label May 19, 2023
Comment on lines 327 to 330
.then(move || {
on_before_rendering.emit(time_info);
drop(_profiler);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional? Just making sure, as I don't know this part, and this change wasn't in PR's description.

Copy link
Contributor Author

@kazcw kazcw May 22, 2023

Choose a reason for hiding this comment

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

Thanks for noticing--fixed the description.

@wdanilo
Copy link
Member

wdanilo commented May 22, 2023

A question – why does it fix #6630? Was that regression introduced by invalidating the graph editor multiple times per frame and this PR fixes that, or something else happened?

@kazcw
Copy link
Contributor Author

kazcw commented May 22, 2023

A question – why does it fix #6630? Was that regression introduced by invalidating the graph editor multiple times per frame and this PR fixes that, or something else happened?

I'll have a look at the increased cost of view initialization when #6630 started and see if there's much room for improvement there.

@kazcw
Copy link
Contributor Author

kazcw commented May 22, 2023

We're spending 14% (475ms) of Orders startup time in 64 calls to widgets' rebuild_tree.

image

While I'm sure we could do better than 7ms per tree, this project only has 12 nodes. We shouldn't need to build 64 widget trees to open it. #6802 will probably help with this: When graph execution changes the data attached to SpanTree nodes instead of changing the whole SpanTree structure, it will be easier to update only widgets that have changed.

@kazcw
Copy link
Contributor Author

kazcw commented May 22, 2023

Actually, widget rebuild is already incremental via a StableSpanIdentity mechanism. Whether #6802 would help with performance here would take more research, though it would help with simplicity.

@Frizi
Copy link
Contributor

Frizi commented May 23, 2023

@kazcw About the widget updates, I don't think that the amount of updates here is in any way alarming. And as you noticed, most of those updates should indeed be done incrementally, due to stable identities. The problem is that creating UI components in general is very expensive right now, partly due to the overhead of creating the display object hierarchy and each component's FRP network (to be more specific, due to the huge amount of tiny allocations needed to do that). I expect that this will improve drastically with the upcoming FRP changes.

Separating the SpanTree (or expression data in general) from execution context data won't necessarily improve the situation, because the work to update the widgets fundamentally has to happen anyway. We would just have two separate streams of data that the tree has to react to. Also, due to how the node expression update is handled, updates with unchanging span tree actually shouldn't even cause the widget tree to be rebuilt. Those unchanged updates are rejected at the graph presenter level.

let displayed_updated = displayed.expression != new_displayed_expr;

Interestingly, the way this is checked is another area for improvement. Building the span-trees only to deep compare them and figure out nothing changed is incredibly wasteful.

@farmaazon
Copy link
Contributor

farmaazon commented May 26, 2023

QA Report: 🔴

Detaching connections going to self port from their source and connecting to another source triggers weird behavior:

qa-graph-updates-2023-05-26_16.32.53.mp4

Often I cannot connect to this self port afterward.

Now, I have actually seen the same issue on the develop once, but in this PR it became 100% reproducible.

@sylwiabr
Copy link
Member

@farmaazon can you re-upload the video?

@farmaazon
Copy link
Contributor

Done. Sorry, I missed it hadn't uploaded.

@kazcw
Copy link
Contributor Author

kazcw commented May 26, 2023

That is #6772, which I don't think is intermittent, except for some timing-sensitivity. It depends on: The target node and port (typically detaching a port that is the receiver of a method, as in your example, causes it), and how long the edge is detached (it breaks when the graph re-evaluates). In a graph similar to your example, I can reproduce it 100% of the time on develop @ b353b35.

@farmaazon
Copy link
Contributor

farmaazon commented May 29, 2023

You are right, it's far more reducible on develop than I've seen during my QA (perhaps I was just lucky). So **the QA turns 🟢 **

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label May 29, 2023
@mergify mergify bot merged commit 7e6a919 into develop May 29, 2023
@mergify mergify bot deleted the wip/kw/graph-coalesce-view-updates branch May 29, 2023 14:39
Procrat added a commit that referenced this pull request May 30, 2023
…le-6756-6804

* develop: (22 commits)
  Coalesce graph editor view invalidations (#6786)
  Append warnings extracted before tail call execution (#6849)
  Detect and override hooks of the same kind (#6842)
  Dynamic app resampling and better performance measurements. (#6595)
  Show spinner when opening/creating a project, take #2 (#6827)
  Infrastructure for testing inter project imports and exports (#6840)
  Only initialise visualisation chooser if it is used. (#6758)
  Allow casting a Mixed column into a concrete type (#6777)
  Stop graph editing when in full-screen visualization mode (#6844)
  Handle `show-dashboard` event (#6837)
  Fix some dashboard issues (#6668)
  Fix JWT leak (#6815)
  Fix "set username" screen (#6824)
  Fallback to opened date when ordering projects (#6814)
  Various test improvements to increase coverage and speed things up (#6820)
  do not activate nested dropdowns together (#6830)
  Clearly select single specialization with enum dispatch pattern (#6819)
  Prevent incorrect application of list widget on incompatible expressions (#6771)
  Update GraalVM to 22.3.1 JDK17 (#6750)
  Import/export syntax error have more specific messages (#6808)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance regression on todays Nightly build in comparison to 2023.5.9 Nightly build
5 participants