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

Keep the list of free channels in sorted order #2151

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

softins
Copy link
Member

@softins softins commented Dec 7, 2021

Short description of changes

Maintain the list of free channel IDs in sorted order, so that the lowest available channel ID will be used for a new client.

The fix has two parts, the first of which is not really necessary, but makes the output of DumpChannels() nicer:

  • Reverse the polarity of the comparison in the binary search, so that the list of active channel IDs has the lowest ID first (this should have been done originally).
  • When moving the channel IDs downwards in the list while freeing a channel, instead of stopping at the end of the active channels, keep going until the correct ordered place is found to insert the freed channel ID.

Context: Fixes an issue?

Fixes #2144

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

No documentation - internal enhancement only.

Status of this Pull Request

Tested and ready to merge.

What is missing until this pull request can be merged?

Nothing

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • 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

@softins
Copy link
Member Author

softins commented Dec 7, 2021

The operation of the list can be observed for testing by temporarily uncommenting the two calls to DumpChannels() in src/server.cpp.

src/server.cpp Outdated
{
int j = i++;
vecChannelOrder[j] = vecChannelOrder[i];
}
// put deleted channel at the end ready for re-use
// put deleted channel in the correct position ready for re-use
Copy link
Collaborator

Choose a reason for hiding this comment

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

This all now seems very cumbersome. If it's going to sit somewhere in vecChannelOrder, why not just leave it where it was?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the binary search relies on all the active channels being listed before all the free ones. All we are changing is how many IDs we shunt down the list when freeing, so that the freed ID is placed in order instead of just at the start of the free list. Doesn't seem cumbersome to me at all :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

So you're saying it was (allocated | free):

1 2 3 4 5   ||
1 3 4 5 | 2 ||
1 3 4 | 5 2 ||
3 4 | 1 5 2 ||

and now it's

1 2 3 4 5   ||
1 3 4 5 | 2 ||
1 3 4 | 2 5 ||
3 4 | 1 2 5 ||

? I think the comment doesn't explain anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly that. I'll change the comment to say "put deleted channel in the vacated position ready for re-use"

@softins softins force-pushed the channel-allocation branch from f632499 to ff62760 Compare December 8, 2021 12:59
src/server.cpp Outdated Show resolved Hide resolved
@softins softins force-pushed the channel-allocation branch from ff62760 to 7bc861b Compare December 8, 2021 19:57
@softins softins requested a review from gilgongo December 8, 2021 23:03
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Looks ok.

@softins softins merged commit 081cce4 into jamulussoftware:master Dec 11, 2021
@softins softins deleted the channel-allocation branch January 11, 2022 12:17
@gilgongo gilgongo added this to the 3.8.2 milestone Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server channel numbers are not reset if last client to disconnect is > 0
4 participants