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

Fixes an unexpected performance regression in SpectrumView #2267

Merged

Conversation

crsib
Copy link
Member

@crsib crsib commented Dec 10, 2021

After merging the spectrum brush branch - a new hot path was accidentally added to spectrum rendering.

  1. std::round is expensive as is. The implementation is complex and designed to handle different situations.
  2. By mistake the following chain was introduced: round(float)->int->double->round(double)->int

The new implementation uses lrintf. By default, it rounds to nearest, and ties are rounded to even. This does not match the behavior std::round entirely, but given that it only slightly affects the visuals, that shouldn't be a problem

Resolves: (direct link to the issue)

(short description of the changes and the motivation to make the changes)

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

After merging the spectrum brush branch - a new hot path was accidentally added to spectrum rendering.

1. std::round is expensive as is. The implementation is complex and designed to handle different situations.
2. By mistake the following chain was introduced: round(float)->int->double->round(double)->int

The new implementation uses `lrintf`. By default, it rounds to nearest, and ties are rounded to even. This does not match the behavior `std::round` entirely, but given that it only slightly affects the visuals, that shouldn't be a problem
@crsib crsib requested a review from Paul-Licameli Dec 10, 2021
@crsib crsib added this to In progress in Sprint 10 - Enhancements&Bug fixes via automation Dec 13, 2021
@crsib crsib moved this from In progress to Review in progress in Sprint 10 - Enhancements&Bug fixes Dec 13, 2021
@Paul-Licameli
Copy link
Member

Paul-Licameli commented Dec 14, 2021

The original std::round by Edward is replaced by lrintf which should make a difference only with exact values at the halves, and I don't care how the difference affects the display.

Rounding values that were already rounded was a supid mistake of my own!

Sprint 10 - Enhancements&Bug fixes automation moved this from Review in progress to Reviewer approved Dec 14, 2021
@Paul-Licameli Paul-Licameli merged commit 6ff0e9a into audacity:release-3.1.3 Dec 14, 2021
4 checks passed
Sprint 10 - Enhancements&Bug fixes automation moved this from Reviewer approved to Ready for QA Dec 14, 2021
@Penikov Penikov moved this from Ready for QA to In QA in Sprint 10 - Enhancements&Bug fixes Dec 14, 2021
@crsib crsib deleted the spectrum_view_performance branch Dec 15, 2021
@crsib
Copy link
Member Author

crsib commented Dec 15, 2021

Rounding values that were already rounded was a supid mistake of my own!

What is stupid is how std::round is defined for the integers:

A set of overloads or a function template accepting an argument of any integral type. Equivalent to 2), 6), or 10), respectively (the argument is cast to double).

I would expect it to be noop

@Penikov Penikov moved this from In QA to Done in Sprint 10 - Enhancements&Bug fixes Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants