Skip to content

access QPainterPathPrivate for faster arrayToQPath#2324

Merged
j9ac9k merged 3 commits intopyqtgraph:masterfrom
pijyoi:qpath_guts
Jun 11, 2022
Merged

access QPainterPathPrivate for faster arrayToQPath#2324
j9ac9k merged 3 commits intopyqtgraph:masterfrom
pijyoi:qpath_guts

Conversation

@pijyoi
Copy link
Copy Markdown
Contributor

@pijyoi pijyoi commented Jun 5, 2022

This PR allows populating QPainterPath without having to go through the serialization / deserialization steps.
Where the serialization / deserialization could still be considered a public API (although the binary serialized format is not publicly documented), this method clearly strays into the Qt private internals.

To see this in action, run PlotSpeedTest.py and switch to 'array' (or 'pairs') mode. Then toggle the enableExperimental checkbox to activate this new method.
On my system, this brings array's performance on par with all's performance. (This raises the possibility of making all connect kinds be implemented simply in terms of array)

Obviously, the Qt library is free to change its internal private implementation at any time, which would break this method.

Not tested on 32-bit systems. The ctypes used should be correct on both 32-bit / 64-bit though.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 5, 2022
View reviewed changes
pyqtgraph/functions.py
ds >> path
if isinstance(backstore, QtCore.QByteArray):
ds = QtCore.QDataStream(backstore)
ds >> path

Check notice

Code scanning / CodeQL

Statement has no effect

This statement has no effect.
@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Jun 5, 2022

(This raises the possibility of making all connect kinds be implemented simply in terms of array)

Results of running asv on PyQt5.
(For benchmarking purposes, the code was modified to enable this mode without need of enableExperimental)
(To avoid pasting too much output, previous performance numbers can be found at #2036.)
This does show that all connect kinds could indeed, with this method, be implemented more efficiently in terms of array.

[ 37.50%] ··· arrayToQPath.TimeSuiteAllFinite.time_test                                                              ok
[ 37.50%] ··· ========= ============ ============= ============ ============
              --                               param2
              --------- ----------------------------------------------------
                param1      all          finite       pairs        array
              ========= ============ ============= ============ ============
                10000     104±5μs      113±0.9μs    33.9±0.3μs   35.7±0.2μs
                100000   2.95±0.2ms   3.01±0.03ms    871±20μs     882±4μs
               1000000   28.9±0.4ms    29.9±0.4ms   16.4±0.4ms   14.9±0.7ms
              ========= ============ ============= ============ ============

[ 50.00%] ··· arrayToQPath.TimeSuiteWithNonFinite.time_test                                                          ok
[ 50.00%] ··· ========= ============ ============= ============= ============
              --                                param2
              --------- -----------------------------------------------------
                param1      all          finite        pairs        array
              ========= ============ ============= ============= ============
                10000    128±0.5μs      144±1μs      129±0.6μs    131±0.5μs
                100000   3.82±0.2ms   1.67±0.04ms   2.50±0.02ms   2.73±0.2ms
               1000000    34.3±1ms     17.7±0.4ms    29.2±0.2ms   26.2±0.1ms
              ========= ============ ============= ============= ============

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 5, 2022

Hi @pijyoi on my 2019 Macbook Pro I recorded a change of performance of 330 fps -> 345 fps with this change. This is substantial enough that it is worth adopting IMO.

As with the risk of Qt changing its internals, we should likely add tests to ensure that the paths generated like you propose here are equivalent to paths generated via the "slow" methods so that if things do change, our test suite catches it.

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Jun 5, 2022

on my 2019 Macbook Pro I recorded a change of performance of 330 fps -> 345 fps with this change.

On my Windows laptop, I got a rather more substantial increase of 360 --> 450.
On the same laptop with Linux running on WSLg, I got 265 --> 310.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 5, 2022 via email

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 11, 2022

This LGTM thanks for the PR and thanks for testing!

@j9ac9k j9ac9k merged commit a237b6e into pyqtgraph:master Jun 11, 2022
@pijyoi pijyoi deleted the qpath_guts branch June 12, 2022 00:15
@pijyoi pijyoi mentioned this pull request Jun 17, 2022
Closed
@j9ac9k j9ac9k mentioned this pull request Jun 18, 2022
Merged
@ptapping
Copy link
Copy Markdown

ptapping commented Jun 21, 2022

Results from a quick test of PlotSpeedTest.py on my AMD Ryzen 7 4700U, PySide6 on Manjaro Linux. With window maximised and for connect mode without -> with experimental enabled:

all:   420 -> 420
pairs: 400 -> 520
array: 340 -> 420

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 21, 2022

Results from a quick test of PlotSpeedTest.py on my AMD Ryzen 7 4700U, PySide6 on Manjaro Linux. With window maximised and for connect mode without -> with experimental enabled:


all:   420 -> 420

pairs: 400 -> 520

array: 340 -> 420

Woah that pairs speed! That's for testing and sharing your results

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

@pijyoi @j9ac9k @ptapping