Skip to content

Add iterations argparse argument to benchmarks#2418

Merged
j9ac9k merged 1 commit intopyqtgraph:masterfrom
j9ac9k:try-clean-closure-after-n-updates
Jun 27, 2023
Merged

Add iterations argparse argument to benchmarks#2418
j9ac9k merged 1 commit intopyqtgraph:masterfrom
j9ac9k:try-clean-closure-after-n-updates

Conversation

@j9ac9k
Copy link
Copy Markdown
Member

@j9ac9k j9ac9k commented Sep 17, 2022 •
edited
Loading

Add --iterations argument to the benchmark examples allowing the benchmark to exit on its own cleanly after n updates. This is useful for profilers which need the application to exit before computing results.

EDIT: Below was the original PR post, decided to walk back the scope of this PR by scrapping the changes to try and compute the setData + paint duration

Add functionality to allow for the benchmark to stop after a specific number of iterations. By default, will run indefinitely as it currently does, however, if an iteration argument is passed with a numerical value, the update timer will stop, and the example will exit cleanly.

This is my first attempt at using QSignalMapper, while I would normally use self.sender(), the update method is a function, not a method, where self is not an argument being passed in.

QEventLoop to block the python code until the paint function finishes. This should provide for a far more accurate duration calculation than QApplication.processEvents()

Redo the FPS calculation to take into account the entirety of the Qt EventLoop, not just the call to paint.

@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch 5 times, most recently from f50602e to 3095691 Compare September 17, 2022 23:42
@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch 5 times, most recently from ed363de to b8d64ee Compare September 30, 2022 03:47
@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch 3 times, most recently from bd24ccc to 71cf699 Compare February 11, 2023 05:45
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 11, 2023
View reviewed changes
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Feb 11, 2023

I should probably update VideoSpeedTest.py in a similar fashion.

@ntjess
Copy link
Copy Markdown
Contributor

ntjess commented Feb 11, 2023

Something scary that happens on Windows 11

image

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Feb 11, 2023

Hmm, I'll test on windows and see if I can replicate.

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Feb 11, 2023

@ntjess what Qt bindings were you using to create this issue?

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Feb 11, 2023

Also on ScatterPlotSpeedTest.py when I switch to simulating hover events, I get the following console output:

QEventLoop::exec: instance 0x6000013995e0 has already called exec()

Debating if this is "ok" or if I should add a little more logic to try and account for this.

@ntjess
Copy link
Copy Markdown
Contributor

ntjess commented Feb 11, 2023

@ntjess what Qt bindings were you using to create this issue?

PyQt5 5.15.7 Qt 5.15.2

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Feb 12, 2023

I am getting the same error on Windows 11, Qt5 and Qt6.

The following 2 changes seems to make things work.

flags = QtCore.QEventLoop.ProcessEventsFlag.AllEvents   # equivalent to no flags
curve.sigPaintFinished.connect(eventLoop.exit, QtCore.Qt.ConnectionType.QueuedConnection)

The problem as I see it is nested callbacks, the chain of which gets broken by using QueuedConnection.
This is by no means the "right" solution, nor do I know what a "right" solution would be.

I do encounter such issues when using event loops. i.e. wanting to execute some piece of code after the current callback has completed. Qt doesn't have a convenient ready-made "CallAfter" function.

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Feb 12, 2023

If I understand things correctly, using QtCore.Qt.ConnectionType.QueuedConnection would be the same as using QApplication.processEvents(). I want to know how long it takes from the time we call setData to the time the following screen render is done (I know the actual rendering on the screen may happen a bit after the paint method finishes, but that's the best I can do).

Using the QueuedConnection (or QApplication.processEvents() would add the processing of other events in the Qt event loop which I don't really want to muddy the water with the duration benchmark (the FPS calculation should take that into account).

That said, there may not be a good way to capture the performance here short of much fancier signal/slot logic, in which case I should probably unwind the bulk of the diff that this PR has.

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Feb 13, 2023

I'm away from my windows machine for a few days but some potential solutions I thought of

  1. After we collect our measurement info in the update method, we can then run QApplication.processEvents()
  2. After we sigPaintFinished is emitted, we then disconnect (or block) that signal, until the update method is called again, where we then re-connect (or unblock) it.

The first option may impact the fps calculation in a way that is not accurate.

@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch from 71da7f0 to 4c9b926 Compare February 13, 2023 13:08
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Feb 13, 2023

@ntjess would you mind trying the ScatterPlot example again and see if you get the same issue?

@ntjess
Copy link
Copy Markdown
Contributor

ntjess commented Feb 14, 2023

Still happens on 4c9b926

@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch 3 times, most recently from d306d06 to f0af833 Compare June 10, 2023 23:05
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 10, 2023
View reviewed changes
@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch 2 times, most recently from 155adc7 to b9e0aba Compare June 10, 2023 23:29
@j9ac9k j9ac9k mentioned this pull request Jun 27, 2023
Merged
@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch 3 times, most recently from 27e0191 to 8080478 Compare June 27, 2023 05:03
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 27, 2023

Ok, after merging #2752 decided to revisit this and re-limit the scope.

This PR only adds the --iterations argument to the benchmarks, allowing users to specify the number of iterations before the benchmark exits automatically.

This is useful when running a benchmark within a profiler, and developers need the application to exit before you can view the results.

@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch from 8080478 to 96bdfc6 Compare June 27, 2023 05:19
@j9ac9k j9ac9k changed the title Add iterations argparse argument to PlotSpeedTest Add iterations argparse argument to benchmarks Jun 27, 2023
@j9ac9k
Add iterations argparse argument to PlotSpeedTest
c4e8c5f 
Add functionality to allow for the benchmark to stop after a specific
number of iterations.  By default, will run indefinitely as it currently
does, however if an iteration argument is passed with a numerical value,
the update timer will stop, and the example will exit cleanly.
@j9ac9k j9ac9k force-pushed the try-clean-closure-after-n-updates branch from 96bdfc6 to c4e8c5f Compare June 27, 2023 05:22
@j9ac9k j9ac9k merged commit 97856c1 into pyqtgraph:master Jun 27, 2023
@j9ac9k j9ac9k deleted the try-clean-closure-after-n-updates branch June 27, 2023 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@j9ac9k @ntjess @pijyoi