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 several performance problems found with Valgrind #2579

Merged
merged 1 commit into from
Mar 6, 2016

Conversation

michaelgregorius
Copy link
Contributor

Removes some repeated calls to Qt's font layouting:

  • FxLine::drawFxLine now uses a QStaticText instance to render the channel names. The method drawFxLine was turned into a member method so that the QStaticText member can be accessed.
  • Removing the overridden method ComboBox::sizeHint.

Unifies Mixer::peakValueLeft and Mixer::peakValueRight into
Mixer::getPeakValues so the array is only iterated once.

@Fastigium
Copy link
Contributor

Did a quick test, and it looks good to me 👍. Nice optimization work! Minor nitpicking: https:/LMMS/lmms/pull/2579/files#diff-b6cdb0811e7a10721f0bfb60ea98f525R180 lacks the spaces between parentheses and arguments, but I believe that particular style guideline isn't observed that strictly anymore.

@tresf
Copy link
Member

tresf commented Feb 20, 2016

I believe that particular style guideline isn't observed that strictly anymore.

Correct. We're slowly moving away from it, so we try to do our best to match the style of the item we're modifying until we have time to clean it all up. 👍

@@ -314,8 +314,7 @@ class EXPORT Mixer : public QObject
m_playHandleRemovalMutex.unlock();
}

static float peakValueLeft( sampleFrame * _ab, const f_cnt_t _frames );
static float peakValueRight( sampleFrame * _ab, const f_cnt_t _frames );
void getPeakValues( sampleFrame * _ab, const f_cnt_t _frames, float & peakLeft, float & peakRight );
Copy link
Member

Choose a reason for hiding this comment

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

Function should be const.

@Wallacoloo
Copy link
Member

Works well. Look into marking any const methods (I left code comments), and then it should be good to merge, I'd say.

Removes some repeated calls to Qt's font layouting by using QStaticText
in FxLine and removing the overridden method ComboBox::sizeHint.

Unifies Mixer::peakValueLeft and Mixer::peakValueRight into
Mixer::getPeakValues so the array is only iterated once.
michaelgregorius added a commit that referenced this pull request Mar 6, 2016
Fixes several performance problems found with Valgrind
@michaelgregorius michaelgregorius merged commit 2e2abdf into LMMS:master Mar 6, 2016
@michaelgregorius michaelgregorius deleted the performance branch March 6, 2016 17:49
@tresf
Copy link
Member

tresf commented Mar 6, 2016

Hmm... Travis says this errored, but the green check is above...

Even stranger, the build actually compiled, but says exit 1. Safe to ignore I think, but we'll keep our eye on it. Re-launched the Travis process just incase.

screen shot 2016-03-06 at 2 27 36 pm

[100%] Building CXX object tests/CMakeFiles/tests.dir/tests_automoc.cpp.o
[100%] Linking CXX executable tests
[100%] Built target tests
>> Will run 1 test suites 
********* Start testing of ProjectVersionTest *********
Config: Using QTest library 4.8.7, Qt 4.8.7
PASS   : ProjectVersionTest::initTestCase()
PASS   : ProjectVersionTest::test()
PASS   : ProjectVersionTest::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped
********* Finished testing of ProjectVersionTest *********
<< 0 out of 1 test suites failed. 
The command "if [[ $TARGET_OS != win* ]]; then make tests && ./tests/tests; fi;" exited with 0.
Done. Your build exited with 1.

@michaelgregorius
Copy link
Contributor Author

@tresf That's strange indeed. I have actually waited for all builds to succeed before merging this one. Is the attached image from the relaunched build? Only asking because my build was 3510.* and 3510.4 and 3510.5 both say they exited with 0.

@tresf
Copy link
Member

tresf commented Mar 6, 2016

It appears there were two... the pull request and the merge... I don't know and now I have a headache...

On a side note, Travis is known to throw false warnings from time to time. 😕

screen shot 2016-03-06 at 2 42 59 pm
screen shot 2016-03-06 at 2 43 20 pm

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

Successfully merging this pull request may close these issues.

4 participants