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

Implements changes to allow developers more control about event handling #2854

Open
wants to merge 114 commits into
base: next
Choose a base branch
from

Conversation

TheCataliasTNT2k
Copy link

Adds Framework::dispatch_automatically
Makes client::dispatch::update_cache_with_event public, so it can be called manually by developers

GnomedDev and others added 30 commits March 31, 2024 12:52
…erenity-rs#2646)

This avoids having to allocate to store fixed length (replaced with normal
array) or fixed capacity (replaced with `ArrayVec`) collections as vectors for
the purposes of putting them through the `Request` plumbing.

Slight behavioral change - before, setting `params` to `Some(vec![])`
would still append a question mark to the end of the url. Now, we check
if the params array `is_empty` instead of `is_some`, so the question
mark won't be appended if the params list is empty.

Co-authored-by: Michael Krasnitski <42564254+mkrasnitski@users.noreply.github.com>
These are unnecessary. Accepting `impl Into<Arc<T>>` allows passing either `T` or `Arc<T>`.
This trades a heap allocation for messages sent along with thread
creation for `Message`'s inline size dropping from 1176 bytes to 760
bytes,
…l models (serenity-rs#2656)

This shrinks type sizes by a lot; however, it makes the user experience slightly
different:

- `FixedString` must be converted to String with `.into()` or `.into_string()`
  before it can be pushed to, but dereferences to `&str` as is.
- `FixedArray` must be converted to `Vec` with `.into()` or `.into_vec()`
  before it can be pushed to, but dereferences to `&[T]` as is.

The crate of these types is currently a Git dependency, but this is fine for
the `next` branch. It needs some basic testing, which Serenity is perfect for,
before a release will be made to crates.io.
…enity-rs#2668)

This commit:

- switches from `u64` to `i64` in `CreateCommandOption::min_int_value` and
`CreateCommandOption::max_int_value` to accommodate negative integers in
Discord's integer range (between -2^53 and 2^53). Values outside this
range will cause Discord's API to return an error.
- switches from `i32` to `i64` in `CreateCommandOption::add_int_choice` and
`CreateCommandOption::add_int_choice_localized` to accommodate Discord's
complete integer range (between -2^53 and 2^53). Values outside this
range will cause Discord's API to return an error.
This cache was just duplicating information already present in `Guild::members`
and therefore should be removed.

This saves around 700 MBs for my bot (pre-`FixedString`).

This has to refactor `utils::content_safe` to always take a `Guild` instead
of`Cache`, but in practice it was mostly pulling from the guild cache anyway
and this means it is more likely to respect nicknames and other information,
while losing the ability to clean mentions from DMs, which do not matter.
`Embed::fields` previously had to stay as a `Vec` due to `CreateEmbed` wrapping
around it, but by implementing `Serialize` manually we can overwrite the
`Embed::fields` with a normal `Vec`, for a small performance hit on
serialization while saving some space for all stored `Embed`s.
Simply missed these when finding and replacing.
This uses the `bool_to_bitflags` macro to remove boolean (and optional boolean)
fields from structs and pack them into a bitflags invocation, so a struct with
many bools will only use one or two bytes, instead of a byte per bool as is.

This requires using getters and setters for the boolean fields, which changes
user experience and is hard to document, which is a significant downside, but
is such a nice change and will just become more and more efficient as time goes
on.
…rs#2681)

This swaps fields that store `Option<Int>` for `Option<NonMaxInt>` where the
maximum value would be ludicrous. Since `nonmax` uses `NonZero` internally,
this gives us niche optimisations, so model sizes can drop some more.

I have had to include a workaround for [serenity-rs#17] in `optional_string` by making my
own `TryFrom<u64>`, so that should be removable once that issue is fixed.

[serenity-rs#17]: LPGhatguy/nonmax#17
A couple of clippy bugs have been fixed and I have shrunk model
sizes enough to make `clippy::large_enum_variant` go away.
A discord bot library should not be using the tools reserved for low
level OS interaction/data structure libraries.
Discord seems to internally default Ids to 0, which is a bug whenever
exposed, but this makes ID parsing more resilient. I also took the
liberty to remove the `From<NonZero*>` implementations, to prevent future
headaches, as it was impossible to not break public API as we exposed
`NonZero` in `*Id::parse`.
…nity-rs#2694)

This,
1. shrinks the size of Request, when copied around, as it doesn't have
to store the max capacity at all times
2. shrinks llvm-lines (compile time metric) for my bot in debug from
`1,153,519` to `1,131,480` as no monomorphisation has to be performed
for `MAX_PARAMS`.
Follow-up to serenity-rs#2694.

When `Request::params` was made into an ArrayVec, the `Option` around it
was removed in order to avoid having to add a turbofish on `None` to
specify the value of `MAX_PARAMS`. Also, `Request::new` also needed to
be changed so that the value of `MAX_PARAMS` could be inferred. Now that
the field is a slice again, we can wrap it in `Option` again (at no cost
to size, thanks to niche opts).

We ensure we never store the redundant `Some(&[])` by checking for an
empty slice and storing `None` instead. This way, we ensure we never
read an empty slice out of the `Some` variant.
The instrument macros generate 2% of Serenity's release mode llvm-lines,
and are proc-macros so hurt compile time in that way, so this limits
them to opt-in. This commit also fixes the issues that the instrument macro
was hiding, such as results that didn't ever error and missing
documentation.
This signature is hard to use as `None` cannot infer the type of the
generic. I also replaced `Option<u8>` with `Option<NonMaxU8>` as it's
more efficient and will make the user think of the maximum value.
…nity-rs#2698)

This removes inefficient `IntoIterator` generics and instead takes what is
actually required. I also reworked `reorder_channels` to allow for keeping the
generic, as it actually does only just need iterator semantics.
Previously, someone assumed that `Ratelimiter` was going to be cloned, so
put a ton of `Arc`s everywhere. This was unneeded, and before dashmap,
so the buckets were also stored massively inefficiently. This fixes all
that.

I had to shuffle around the `Ratelimit` methods a little bit to return
their sleep time instead of sleeping themselves, so I didn't have to
hold a dashmap lock over an `.await`.
This removes multiple error variants and overall cleans up the codebase
by moving overflow checks into two `ModelError` variants.
Shrinks `size_of::<Error>` from 136 bytes to 64 bytes, while removing unused
variants. This will improve performance for any method returning
`Result<T>` where `T` is less than the size of `Error` as both `Result`'s
`Ok` and `Err` have to be allocated stack space.
The compiler knows best as inlining is quite complicated. This should
help with compile times, significantly.
GnomedDev and others added 14 commits March 31, 2024 12:52
This is possible now more methods are cache-less.
This avoids allocating instances of `serde_json::Value`s.
These were found by auditing .http() calls, instead of just manually
looking at `impl CacheHttp`.
I currently working with wasm on one of my personal projects. I'm would
like to use `serenity` to get access to the discord API response models,
but currently the library can not the compiled to wasm, because the
`tokio/fs` feature is not supported in that target.

I went ahead and removed the need to include that feature by default. By
the looks of it, it only was used with the `builder` and `model`
features, so I conditionally added `tokio/fs` back when those features
are being used (the `model` feature already depends on `builder` so that
doesn't need to be changed).
This has to be on the next branch as reqwest is exposed in a couple
places.
This announcement was missed (around August 2022) and probably the cause
of a lot of the disconnections.
The documentation was never updated after serenity-rs#2662 removed the user cache.
This field is documented as nullable but that isn't reflected in the
model. I also took the opportunity to replace the Option<Vec> with a
default Vec.
This was mistakenly removed in serenity-rs#2278 and wasn't caught because of the
wildcard pattern in the match.
@github-actions github-actions bot added framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate client Related to the `client` module. labels Apr 28, 2024
TheCataliasTNT2k added a commit to RuDrocsid/poise that referenced this pull request Apr 28, 2024
TheCataliasTNT2k added a commit to RuDrocsid/poise that referenced this pull request Apr 28, 2024
TheCataliasTNT2k added a commit to RuDrocsid/poise that referenced this pull request Apr 28, 2024
Makes client::dispatch::update_cache_with_event public, so it can be called manually by developers
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.

I dislike update_cache_with_event becoming public, as it's quite internal. If you could make a nicer API for this that doesn't require making internal modules public, that would resolve this.

@TheCataliasTNT2k
Copy link
Author

TheCataliasTNT2k commented Apr 30, 2024

The only thing in this method I really care about, is to get the returned values.
I do not really want to copy this function to get the FullEvent objects to use them in my library. If this can be achieved in any other way (while not losing the cache functionality) I am happy.

@cheesycod
Copy link
Contributor

This would be useful for me as well

@GnomedDev GnomedDev force-pushed the next branch 3 times, most recently from 7ad183d to 52f2822 Compare May 14, 2024 20:34
@GnomedDev GnomedDev force-pushed the next branch 2 times, most recently from bda6e4e to 6561f4b Compare May 23, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Related to the `client` module. framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet