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

Constructive correction to sluggish expression definition #20149

Closed
wants to merge 9 commits into from

Conversation

markcmiller86
Copy link
Member

@markcmiller86 markcmiller86 commented Dec 17, 2024

Description

Its hard to say this resolves #18925 and #19488 but it does improve performance a ton. It eliminates order n^2 behavior because it bypasses the GUI temporarily.

It introduceds two new CLI methods; SuspendGUIUpdates() and ResumeGUIUpdates(). At present, these methods are really specific to expression definitions. I mean, that is all these methods wind up suspending in the way of GUI updates is expression updates. Other GUI updates proceed normally.

On my mac, 300 instances in #19488 was taking forever...minutes. I didn't wait. But, with these methods, it takes 6 seconds. And, if I double that count to 600 instances, the time approximately doubles too. So, no more n^2 behavior.

Type of change

  • Bug fix~~
  • [ ] New feature
  • [ ] Documentation update
  • [ ] Other

How Has This Been Tested?

Reminders:

  • Please follow the style guidelines of this project.
  • Please perform a self-review of your code before submitting a PR and asking others to review it.
  • Please assign reviewers (see VisIt's PR procedures for more information).

Checklist:

  • I have commented my code where applicable.~~
  • I have updated the release notes.~~
  • I have made corresponding changes to the documentation.~~
  • [ ] I have added debugging support to my changes.
  • I have added tests that prove my fix is effective or that my feature works.~~
  • [ ] I have confirmed new and existing unit tests pass locally with my changes.
  • [ ] I have added new baselines for any new tests to the repo.
  • I have NOT made any changes to protocol or public interfaces in an RC branch.~~


**Description:**

SuspendGUIUpdates temporarily suspends updating the GUI after certain CLI
Copy link
Contributor

Choose a reason for hiding this comment

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

Does our single line per sentence guidelines apply in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes and no. The tools have been upgraded to wrap the text that gets put into MethodDoc.C. So, now, yes, you can avoid having to wrap lines manually in this file. But, that did not used to be the case. It used to be that MethodDoc.C was a verbatim copy of the lines here so that it would display properly in the python interpreter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was asking because we do single-sentence per line to make it easier to check diffs during code review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed now.

Copy link
Member

@JustinPrivitera JustinPrivitera left a comment

Choose a reason for hiding this comment

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

Remove your cmake file?

@@ -1,14 +1,14 @@
#/Users/miller86/visit/third_party/3.3.0/cmake/3.24.3/darwin-x86_64/bin/cmake
Copy link
Member

Choose a reason for hiding this comment

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

should scratlantis.cmake be in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for doing this by the way. This is a good band-aid to have while we try to figure out what is going wrong under the hood.

Copy link
Member Author

Choose a reason for hiding this comment

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

should scratlantis.cmake be in this PR?

Well, I have to update it before doing any more work on develop so I just thought I'd let it go along for the ride. Ok?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

biagas
biagas previously approved these changes Dec 18, 2024
@markcmiller86
Copy link
Member Author

Unfortunately, due to asynchrony between GUI and CLI, the timing values that get printed by the CLI don't reflect the reality of what happens in the GUI. The CLI returns very quickly. But, the GUI remains unresponsive for a long time as all the expression list is updated upon resuming. So, I think this doesn't quite fix things.

What would fix things I think is to disable variablemenupopulator at this time too.

@markcmiller86
Copy link
Member Author

So, VariableMenuPopulator is playing a big role here too. That method does a crap TON of work to clear and repopulate the menus as a new variable is added. I believe it is this part of things that is leadering to the exponential cost of definining new expressions.

We Notify() the ExpressionList object (shared between viewer, gui and cli) each time a new expression is added. This causes VariableMenuPopulator to do its work each new expression.

We need one of two things. A way to do bulk addition/deletion of variables (so, everything gets added to the list of variables/expressions and VariableMenuPopulator updates just once with all the new stuff) or a way to suspend/resume VariableMenuPopulator during heavy traffic times.

@markcmiller86
Copy link
Member Author

Unfortunately, I am going to close this without merging. It does NOT actually fix the issue. The GUI will still be tied up for a very long time as all the asynchronous activity pushed from the CLI drains into it.

We really need to just fix the GUI's variable menu population activities.

@markcmiller86
Copy link
Member Author

@JustinPrivitera now, I guess your remark about scratlantis.cmake being in this PR is much more significant than I first reacted.

@markcmiller86
Copy link
Member Author

FYI...I did push some last minute work to the branch associated with this PR primarily for posterity's sake if we ever resurrect this approach in the future...42da2b4

@markcmiller86 markcmiller86 reopened this Jan 7, 2025
@markcmiller86 markcmiller86 dismissed stale reviews from JustinPrivitera and biagas via 42da2b4 January 7, 2025 00:20
@markcmiller86 markcmiller86 deleted the feat-mcm86-17dec24-suspnd-gui-updates branch January 21, 2025 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants