Improve algorithm to find in use channels in O(n) instead O(n²) #2738

ann0see · 2022-07-17T18:55:21Z

Short description of changes

This replaces the O(n²) approach to find channels in use with a simpler O(n) approach.

Probably there are other places which would benefit from a more efficient approach too.

CHANGELOG: Client: Improve performance of GUI when someone join or leave a server.

Context: Fixes an issue?

Related to: #2737 (comment)

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Close performance evaluation and (in depth) testing:

Does the ordering get messed up? (it seems ok for me for a small number of clients)
Are the faders hidden correctly at all times (didn't test in real world scenario yet)

What is missing until this pull request can be merged?

Testing

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want (at least I don't see any bad changes on joins/leaves. It must however be tested)
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

src/audiomixerboard.cpp

ann0see · 2022-07-17T18:59:37Z

src/audiomixerboard.cpp

            {
+                // if current fader is not used, hide it


Seems a bit redundant here

ann0see · 2022-07-17T19:07:13Z

Own channel first doesn't seem to work correctly at the moment: If we set the own channel checkbox, nothing happens at first. You need to set a ordering first.

It's not reproducible; At least it only appeared on the first launch of the app. Also setting ini files doesn't seem to get the issue back.

src/audiomixerboard.cpp

pljones · 2022-07-18T06:31:17Z

Note that a lot of the code in audiomixerboard.cpp (GUI only) needs moving into a non-GUI class, so that state is not being maintained in the GUI. Any large scale restructuring should take that into account as one of its (main) goals.

pljones · 2022-07-18T17:47:31Z

Own channel first doesn't seem to work correctly at the moment: If we set the own channel checkbox, nothing happens at first. You need to set a ordering first.

It's not reproducible; At least it only appeared on the first launch of the app. Also setting ini files doesn't seem to get the issue back.

Doesn't surprise me. A lot of the code is impentrable. Hence I'd rather a full refactor than trying to "fix in place".

ann0see · 2022-07-18T17:59:43Z

This PR is mainly about implementing the algorithm we used for the gain reset here in the GUI too; not that much about a separation into a second class. I still think it's worth merging this one? However, what exactly do you want to get extracted into another class?

ann0see · 2022-07-18T18:03:47Z

Ok. I can confirm that own fader first is broken by this change. This needs more investigation.

ann0see · 2022-07-18T18:29:59Z

src/audiomixerboard.cpp

+                // check if this is my own fader and set fader property
+                if ( i == iMyChannelID )
+                {
+                    vecpChanFader[i]->SetIsMyOwnFader();


I don't quite get why the new approach breaks the own fader first logic.

I don't see it either. The refactoring looks correct to me.

Are you sure that this PR broke it? I've just tested with 3.8.2 and it seems to be broken there as well, unless I'm testing the wrong thing? :o

I haven't tested anything before that, but I don't remember that own fader first didn't work.

3.8.2 was the first release which contained it. #1809 was the PR. I'm currently looking into it and will open a separate issue if necessary.

I thought I had reproduced the own-fader-first-breakage with this PR and then with a clean 3.8.2 build. Now I can no longer reproduce it either way. Everything works fine and I'm not sure what I'm doing differently now.

@ann0see Can you share how you made it break?

I think it's related to a fresh start without sorting and then enabling own fader first.

Not sure, but the following could lead to the issue (untested):

Connect to a crowded server without sorting

Wait until someone joins

Set own fader first

Observe you aren't first

hoffie · 2022-07-18T20:31:58Z

Note that a lot of the code in audiomixerboard.cpp (GUI only) needs moving into a non-GUI class, so that state is not being maintained in the GUI. Any large scale restructuring should take that into account as one of its (main) goals.

Yep. In the mean time, we should merge any iterative improvements such as this PR (once proven correct... ;)) neverthless, in my opinion.

src/audiomixerboard.cpp

ann0see · 2022-07-22T18:15:58Z

Now I seem to miss a new join (fader doesn't show up) with the same (empty) name. This clearly needs more testing
Actually also 3.8.2dev-fa204ad4 doesn't show the fader, so it seems to be unrelated to my change...

pljones · 2022-07-22T21:14:34Z

Now I seem to miss a new join (fader doesn't show up) with the same (empty) name. This clearly needs more testing Actually also 3.8.2dev-fa204ad4 doesn't show the fader, so it seems to be unrelated to my change...

* |   48168a67 2022-07-18 Merge pull request #2737 from hoffie/fix-lost-gain-changes [Christian Hoffmann]

Could it be related to that?

ann0see · 2022-07-23T20:50:59Z

Probably they're not related to the change in my PR at all (but still need to be investigated.) Can you reproduce any of them on Linux?
(Connect to a server with totally empty client information), wait, join with another client without client information and observe both mixer boards.

ann0see · 2022-07-23T21:02:15Z

Line 1200: why is this not in the mutex? Can we guarantee the number of clients doesn't change before we acquire the lock?

pljones · 2022-07-24T07:26:05Z

Actually, reading your "how to reproduce", I don't think own fader first has ever worked. I generally don't use sort and I turned off the own fader first feature once I realised it didn't work.

ann0see · 2022-07-24T10:47:54Z

Actually I can’t reproduce it anymore. That’s the issue. Sometimes it works, sometimes it doesn’t. Maybe it’s worth looking at the initial PR again.

pljones · 2022-07-24T12:37:11Z

Actually I can’t reproduce it anymore. That’s the issue. Sometimes it works, sometimes it doesn’t. Maybe it’s worth looking at the initial PR again.

Yes, that's the same behaviour I've always had - I see it, then when I try to get a reliable way to reproduce it, I can't.

ann0see · 2022-07-24T12:58:56Z

I just re-checked, at least one of both bugs also shows up on the current 3.8.2 release, so it's unrelated.

ann0see · 2022-07-30T18:50:35Z

@pljones @hoffie I think this PR is ok, the bugs are not related to it.

pljones · 2022-07-30T20:54:58Z

src/audiomixerboard.cpp

+        {
+            if ( iFaderNumber[iChanID] == INVALID_INDEX )
+            {
+                // fader is not used --> hide it


1232/1233 could be rewritten to flow better. "Hide it." "Oh wait!! Before that, just one thing - store the levels. Oh!! Only of certain conditions are met."

So first we store the levels (I don't think the "if some..." is needed - that's the concern of the called method - though maybe the method name needs improving at some point), then we hide the fader. The comment should reflect that.

However..! I don't even think the comment is adding to the code here. "Store...; ...Hide()" is fairly self-explanatory (as the vector name is good).

A simple // fader is not used to describe the case would do.

pljones · 2022-07-30T21:00:46Z

src/audiomixerboard.cpp

+            }
+
+            // current fader is used
+            // check if fader was already in use -> preserve gain value


This comment (line 1241) should move inside the if. Then the "check if" can be removed (and a "not" added). That makes it more or less a duplicate of line 1244. I'd put the "reset everything" part of 1244 on its own line, though.

Also ... "-> preserve gain value" appears to refer to the code at line 1277 - so really shouldn't be here.

pljones · 2022-07-30T21:03:46Z

src/audiomixerboard.cpp

+                vecpChanFader[iChanID]->Reset();
+                vecAvgLevels[iChanID] = 0.0f;
+
+                // check if this is my own fader and set fader property


Don't say "check if" on an if statement. That's what an if statement is. Inside the if -- if the test doesn't make it clear -- explain the state. Here, a comment of "// this is my own fader" would be present. However, it's pretty redundant.

pljones · 2022-07-30T21:08:25Z

src/audiomixerboard.cpp

+                    vecpChanFader[iChanID]->SetIsMyOwnFader();
+                }
+
+                // set and increment the running new client counter needed for sorting (per definition:


This comment doesn't really help...

Something as brief as

// keep track of each new client (for "no sorting" channel sort order, new clients are added to the right-hand side of the mixer)

might do.

src/audiomixerboard.cpp

pljones · 2022-07-30T21:19:52Z

src/audiomixerboard.cpp

                }
            }

-            // if current fader is not used, hide it
-            if ( !bFaderIsUsed )
+            // restore gain (if new name is different from the current one)


This, again, is a duplicate comment -- the comment should be inside the if describing the state there. There's already a comment which does that in the right place.

pljones · 2022-07-30T21:20:48Z

Sorry, just went through in "picky" mode. As there are a lot of changes happening, it might be nice to make the comments clear at the same time.

ann0see · 2022-07-31T14:42:02Z

Sorry, just went through in "picky" mode. As there are a lot of changes happening, it might be nice to make the comments clear at the same time.

No problem. I just added another commit.

pljones · 2022-07-31T16:31:05Z

src/audiomixerboard.cpp

@@ -1210,92 +1210,97 @@ void CAudioMixerBoard::ApplyNewConClientList ( CVector<CChannelInfo>& vecChanInf

        // search for channels with are already present and preserve their gain


^with^which

pljones · 2022-07-31T16:33:25Z

Approved but if you want to fix the existing typo, please do.

This replaces the O(n²) approach to find channels in use with a simpler O(n) approach.

Co-Authored-By: Peter L Jones <pljones@users.noreply.github.com>

ann0see requested review from pljones and hoffie July 17, 2022 18:56

ann0see commented Jul 17, 2022

View reviewed changes

hoffie reviewed Jul 17, 2022

View reviewed changes

src/audiomixerboard.cpp Outdated Show resolved Hide resolved

ann0see force-pushed the refactor/ImproveAudioMixBoard branch from 524ff43 to 2e078e4 Compare July 18, 2022 17:30

ann0see commented Jul 18, 2022

View reviewed changes

hoffie reviewed Jul 18, 2022

View reviewed changes

src/audiomixerboard.cpp Show resolved Hide resolved

hoffie reviewed Jul 18, 2022

View reviewed changes

src/audiomixerboard.cpp Outdated Show resolved Hide resolved

src/audiomixerboard.cpp Outdated Show resolved Hide resolved

ann0see force-pushed the refactor/ImproveAudioMixBoard branch 3 times, most recently from 728b2c0 to 0eeae20 Compare July 22, 2022 18:14

ann0see force-pushed the refactor/ImproveAudioMixBoard branch 2 times, most recently from 6d94d5b to 562acb5 Compare July 22, 2022 18:22

ann0see mentioned this pull request Jul 24, 2022

Own fader to first position #1809

Merged

ann0see marked this pull request as ready for review July 27, 2022 19:15

pljones reviewed Jul 30, 2022

View reviewed changes

src/audiomixerboard.cpp Show resolved Hide resolved

pljones reviewed Jul 30, 2022

View reviewed changes

ann0see force-pushed the refactor/ImproveAudioMixBoard branch 2 times, most recently from 00bd916 to e2f4344 Compare July 31, 2022 13:32

ann0see force-pushed the refactor/ImproveAudioMixBoard branch from e2f4344 to 997a387 Compare July 31, 2022 14:42

ann0see requested a review from pljones July 31, 2022 14:43

ann0see added this to the Release 3.9.1 milestone Jul 31, 2022

pljones reviewed Jul 31, 2022

View reviewed changes

pljones approved these changes Jul 31, 2022

View reviewed changes

ann0see force-pushed the refactor/ImproveAudioMixBoard branch from 997a387 to 824cfed Compare July 31, 2022 17:11

ann0see and others added 2 commits August 5, 2022 19:55

Improve algorithm to find in use channels

afd2d8e

This replaces the O(n²) approach to find channels in use with a simpler O(n) approach.

Refactoring: Improve/Remove comments

603cea1

Co-Authored-By: Peter L Jones <pljones@users.noreply.github.com>

ann0see force-pushed the refactor/ImproveAudioMixBoard branch from 824cfed to 603cea1 Compare August 5, 2022 17:55

hoffie approved these changes Aug 7, 2022

View reviewed changes

ann0see merged commit 707634e into jamulussoftware:master Aug 7, 2022

ann0see deleted the refactor/ImproveAudioMixBoard branch August 7, 2022 20:16

ann0see mentioned this pull request Aug 9, 2022

Newly connected empty clients don't show up on old clients #2754

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve algorithm to find in use channels in O(n) instead O(n²) #2738

Improve algorithm to find in use channels in O(n) instead O(n²) #2738

ann0see commented Jul 17, 2022 •

edited

Loading

ann0see Jul 17, 2022

ann0see commented Jul 17, 2022 •

edited

Loading

pljones commented Jul 18, 2022

pljones commented Jul 18, 2022

ann0see commented Jul 18, 2022

ann0see commented Jul 18, 2022

ann0see Jul 18, 2022

hoffie Jul 18, 2022

ann0see Jul 18, 2022

hoffie Jul 18, 2022

hoffie Jul 18, 2022

ann0see Jul 22, 2022

hoffie commented Jul 18, 2022

ann0see commented Jul 22, 2022 •

edited

Loading

pljones commented Jul 22, 2022

ann0see commented Jul 23, 2022 •

edited

Loading

ann0see commented Jul 23, 2022

pljones commented Jul 24, 2022

ann0see commented Jul 24, 2022

pljones commented Jul 24, 2022

ann0see commented Jul 24, 2022

ann0see commented Jul 30, 2022

pljones Jul 30, 2022 •

edited

Loading

pljones Jul 30, 2022 •

edited

Loading

pljones Jul 30, 2022

pljones Jul 30, 2022

pljones Jul 30, 2022

pljones commented Jul 30, 2022

ann0see commented Jul 31, 2022

pljones Jul 31, 2022

pljones commented Jul 31, 2022

		@@ -1210,92 +1210,97 @@ void CAudioMixerBoard::ApplyNewConClientList ( CVector<CChannelInfo>& vecChanInf

		// search for channels with are already present and preserve their gain

Improve algorithm to find in use channels in O(n) instead O(n²) #2738

Improve algorithm to find in use channels in O(n) instead O(n²) #2738

Conversation

ann0see commented Jul 17, 2022 • edited Loading

Checklist

Choose a reason for hiding this comment

ann0see commented Jul 17, 2022 • edited Loading

pljones commented Jul 18, 2022

pljones commented Jul 18, 2022

ann0see commented Jul 18, 2022

ann0see commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoffie commented Jul 18, 2022

ann0see commented Jul 22, 2022 • edited Loading

pljones commented Jul 22, 2022

ann0see commented Jul 23, 2022 • edited Loading

ann0see commented Jul 23, 2022

pljones commented Jul 24, 2022

ann0see commented Jul 24, 2022

pljones commented Jul 24, 2022

ann0see commented Jul 24, 2022

ann0see commented Jul 30, 2022

pljones Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

pljones Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones commented Jul 30, 2022

ann0see commented Jul 31, 2022

Choose a reason for hiding this comment

pljones commented Jul 31, 2022

ann0see commented Jul 17, 2022 •

edited

Loading

ann0see commented Jul 17, 2022 •

edited

Loading

ann0see commented Jul 22, 2022 •

edited

Loading

ann0see commented Jul 23, 2022 •

edited

Loading

pljones Jul 30, 2022 •

edited

Loading

pljones Jul 30, 2022 •

edited

Loading