-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
QML optimization #4339
QML optimization #4339
Conversation
1c2706e
to
01465f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, I made my comments but forgot to submit the review
res/qml/Deck.qml
Outdated
@@ -185,18 +185,15 @@ Item { | |||
|
|||
let minutes = Math.floor(positionSeconds / 60); | |||
let seconds = positionSeconds - (minutes * 60); | |||
let centiseconds = Math.trunc((seconds - Math.trunc(seconds)) * 100); | |||
let deciseconds = Math.trunc((seconds - Math.trunc(seconds)) * 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let deciseconds = Math.trunc((seconds - Math.trunc(seconds)) * 10); | |
const deciseconds = Math.trunc((seconds - Math.trunc(seconds)) * 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
onValueChanged: marker.x = parent.width * value | ||
onValueChanged: { | ||
// Math.round saves tons of CPU by avoiding redrawing for fractional pixel positions. | ||
marker.x = Math.round(parent.width * value * Screen.devicePixelRatio) / Screen.devicePixelRatio; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't the divide make the position fractional again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but only for fractional logical pixels that correspond to real display pixels. So for a scale factor of 2, updates are done for every 0.5 logical pixels, or every real display pixel, but not extremely tiny fractions in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so I guess the speedup comes from the fact that Qt doesn't have to do anti aliasing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't make sense either though.
This feels like it should not be something programming QML should have to concern themselves with. So I feel like this is bug in Qt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, but I know that this little change saves a lot of CPU 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhmm the only other idea I have is that the COs value changed multiple times per frame so the callback is also executed quite often. Since the position is always slightly different, the return value is also different. When rounding we always return the same value in the same pixel, so we often return the same value which enables Qt to do some caching optimization (so only doing work when the value actually changes).
This reduces CPU usage by about 10% on my Intel Core i7 8550U.
01465f5
to
95d7af3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. LGTM
based on #4327