Skip to content

Use VeQuickItem::numberValue to reduce binding overheads #1184

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use VeQuickItem::numberValue to reduce binding overheads #1184

wants to merge 1 commit into from

Conversation

@chriadam
Copy link
Contributor

@chriadam chriadam commented Jun 4, 2024

No description provided.

@chriadam
Copy link
Contributor Author

chriadam commented Jun 4, 2024

This one is risky, and unnecessary - we easily hit the performance requirement (in electrons-paused mode) without this PR.
But, it does seem "nice" to me. Anyway, not for 1.0.0, but worth considering in future.

@chriadam chriadam force-pushed the chriadam/performance-investigation-2-no-clip branch from 0819074 to c0af932 Compare June 4, 2024 04:54
blammit
blammit reviewed Jun 5, 2024
View reviewed changes
components/ThreePhaseBarGauge.qml
property bool inputMode
property int orientation: Qt.Vertical
property bool animationEnabled
property bool inOverviewWidget
Copy link
Contributor

@blammit blammit Jun 5, 2024

Choose a reason for hiding this comment

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

This is not used anywhere?

Copy link
Contributor Author

@chriadam chriadam Jun 5, 2024

Choose a reason for hiding this comment

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

leftovers from when I force pushed the other PR. This PR is targeted at the other branch :-/

@chriadam chriadam force-pushed the chriadam/performance-investigation-2-no-clip branch from c0af932 to 866c191 Compare June 5, 2024 05:11
@chriadam chriadam force-pushed the chriadam/performance-investigation-risky branch from 394da2f to 39940a5 Compare June 5, 2024 05:28
@chriadam chriadam force-pushed the chriadam/performance-investigation-2-no-clip branch 2 times, most recently from d25aab0 to ba3e0b0 Compare June 5, 2024 06:47
@chriadam chriadam force-pushed the chriadam/performance-investigation-risky branch from 39940a5 to 353a587 Compare June 5, 2024 06:49
@chriadam chriadam force-pushed the chriadam/performance-investigation-2-no-clip branch from ba3e0b0 to 223c102 Compare June 5, 2024 07:07
@chriadam chriadam force-pushed the chriadam/performance-investigation-risky branch from 353a587 to f025ac6 Compare June 5, 2024 07:07
blammit
blammit approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@blammit blammit left a comment

Choose a reason for hiding this comment

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

Great!

@chriadam chriadam force-pushed the chriadam/performance-investigation-2-no-clip branch from 223c102 to 3795263 Compare June 5, 2024 07:30
@chriadam chriadam force-pushed the chriadam/performance-investigation-risky branch from f025ac6 to 3c33176 Compare June 5, 2024 07:31
Base automatically changed from chriadam/performance-investigation-2-no-clip to main June 6, 2024 03:36
@chriadam chriadam force-pushed the chriadam/performance-investigation-risky branch from 3c33176 to 11480f0 Compare June 6, 2024 03:38
@blammit
Copy link
Contributor

blammit commented Jun 25, 2024

We might be able to use nullish coalescing to get the same benefit instead, i.e. change

readonly property real frequency: _frequency.value === undefined ? NaN : _frequency.value

to

readonly property real frequency: _frequency.value ?? NaN

as long as that still results in one reading of _frequency.value.

I think we should start using this in the codebase generally to make things more concise.

@chriadam
Copy link
Contributor Author

chriadam commented Jun 25, 2024

Does QML's fast-path JS expression evaluator support nullish coalescing, or does it fall back to full JS context evaluation for expressions containing those?

@blammit
Copy link
Contributor

blammit commented Jun 25, 2024

Does QML's fast-path JS expression evaluator support nullish coalescing, or does it fall back to full JS context evaluation for expressions containing those?

No idea, probably requires hunting through the source to find out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blammit blammit blammit approved these changes

@jpetrell jpetrell Awaiting requested review from jpetrell

@DanielMcInnes DanielMcInnes Awaiting requested review from DanielMcInnes

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

@chriadam @blammit