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

Use RwLock for active collectors collection #2851

Merged
merged 7 commits into from
Apr 26, 2024

Conversation

TheCataliasTNT2k
Copy link

One of the main functions used a Mutex to guard a shared variable.
This may not impact small bots, but can significantly slow down bots, which have a huge volume of GatewayEvents.
This Pullrequest replaces these Mutexes with a RwLock, which allows multiple simultaneous readers.
This should not break any existing code.

@github-actions github-actions bot added client Related to the `client` module. gateway Related to the `gateway` module. labels Apr 25, 2024
@GnomedDev
Copy link
Member

This is a breaking change, please re-target to the next branch.

@TheCataliasTNT2k TheCataliasTNT2k changed the base branch from current to next April 25, 2024 22:48
@arqunis arqunis added enhancement An improvement to Serenity. breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users dependencies Related to Serenity dependencies. labels Apr 25, 2024
@TheCataliasTNT2k
Copy link
Author

I do not know, what this should break, I tested it with current and it worked without any other additions, but I changed it.

@GnomedDev
Copy link
Member

let mutex = Arc::new(tokio::sync::Mutex::new("".to_string()));
Shard::new(mutex, "", shard_info, GatewayIntents::all(), None).await?;

This is an example of user code that would break if this PR was merged. Breaking changes means any public API change that stops compilation, with few exceptions. This PR now needs rebasing to drop the extra commits from current and to solve the conflicts.

@TheCataliasTNT2k
Copy link
Author

Ah, I forgot about the ws_url, I see. I will hust rebase it onto next, this should be enough.

The Mutex can only be locked once, the RwLock allows multiple readers to get a handle.
This is especially needed in functions, which have a high throughput.
@TheCataliasTNT2k
Copy link
Author

I now got a lot of "Unresolved reference: instrument", but I ignored them, because they already existed on the branch.

@GnomedDev
Copy link
Member

Disable the tracing_instrument feature in your builds, it is currently broken, that will get those errors to go away.

@TheCataliasTNT2k
Copy link
Author

I did not get build errors, cargo test --features full runs successfully, jut my IDE is very unhappy.

@GnomedDev
Copy link
Member

Yes, but you have probably enabled --all-features in your IDE, so it enables the broken feature.

@TheCataliasTNT2k
Copy link
Author

I will let rustfmt fix the linter issue.
What to do about the other linter issue? Should I really create a new type alias for that?

src/gateway/bridge/shard_runner.rs Outdated Show resolved Hide resolved
src/gateway/bridge/shard_runner.rs Show resolved Hide resolved
src/gateway/bridge/mod.rs Outdated Show resolved Hide resolved
src/gateway/bridge/mod.rs Outdated Show resolved Hide resolved
Simplify CollectorCallback type
Change pointer conversion
@github-actions github-actions bot removed the client Related to the `client` module. label Apr 26, 2024
src/collector.rs Outdated Show resolved Hide resolved
src/gateway/bridge/mod.rs Outdated Show resolved Hide resolved
@TheCataliasTNT2k
Copy link
Author

Too stupid to see errors sometimes...

@GnomedDev GnomedDev changed the title Changes Mutex to parking_lot::RwLock in a few places Use RwLock for active collectors collection Apr 26, 2024
Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the PR!

@GnomedDev GnomedDev merged commit 83ba59e into serenity-rs:next Apr 26, 2024
21 checks passed
@TheCataliasTNT2k
Copy link
Author

Thanks for the help!

GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request May 14, 2024
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Jun 9, 2024
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Jun 22, 2024
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jul 29, 2024
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users dependencies Related to Serenity dependencies. enhancement An improvement to Serenity. gateway Related to the `gateway` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants