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

Replace Vec and String with FixedArray and FixedString for all models #2656

Merged
merged 1 commit into from
Dec 10, 2023

Conversation

GnomedDev
Copy link
Member

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

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

Currently this is a git dependency, but I hope that is fine for next. I want to get some basic testing in serenity before I push to crates.io.

@github-actions github-actions bot added framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate model Related to the `model` module. client Related to the `client` module. builder Related to the `builder` module. cache Related to the `cache`-feature. utils Related to the `utils` module. gateway Related to the `gateway` module. examples Related to Serenity's examples. labels Dec 6, 2023
@GnomedDev GnomedDev marked this pull request as draft December 6, 2023 10:48
@GnomedDev GnomedDev force-pushed the fixedstring branch 8 times, most recently from d14248d to f34d38a Compare December 6, 2023 13:04
@mkrasnitski
Copy link
Collaborator

Without taking a look at the changes, what's the benefit of these types over ArrayVec and ArrayString from the arrayvec crate?

@GnomedDev
Copy link
Member Author

ArrayVec and ArrayString are both inline allocated and have a compile time max capacity
FixedArray and FixedString are both heap allocated, have a max length of u32::MAX, and don't store capacity

@mkrasnitski
Copy link
Collaborator

Looking at the lib, it seems like there are no mutable APIs for either of the types? If that's the case, why not then use boxed arrays? At the very least, at a cursory glance I'm not convinced the FixedString API is all that sound - I think it would be best to replace that with Box<str>, which is guaranteed sound.

@GnomedDev
Copy link
Member Author

Can you read the module level documentation of the lib?

These can be thought of as Box<[T]> and Box<str>, except the length is denoted as a [u32].

As for the soundness, there is very little unsafe in the library, and you are free to audit it, but it's pretty simple and I can't see many failure points.

@ultrabear
Copy link

theres a bit of UB (that i can see) that is amendable by erroring on platforms where pointer width is not 32 or 64 bit

UB in question

    fn len(&self) -> usize {
        self.small_len() as usize
    }

@GnomedDev
Copy link
Member Author

Can you move that to an issue on the repo, instead of here, although thanks for letting me know, I'll see if I can restriction lint out as entirely.

@mkrasnitski
Copy link
Collaborator

Can you read the module level documentation of the lib?

Apologies about that, I wasn't being very careful with my skimming.

With regards to the size of the struct, we're already saving a usize for capacity, which we can assume is 8 bytes. However, an extra 4 bytes of size is not worth the extra maintenance burden IMO, considering this type would be included in basically every model type, and in some cases be user facing. The deref impl definitely helps, but IMO it would still be too exotic.

@GnomedDev
Copy link
Member Author

I know I've floated the idea before, but how about an unstable-optimisations feature flag to swap implementations for this stuff around. It could be Box for most users and FixedString for crazy people.

@mkrasnitski
Copy link
Collaborator

I know I've floated the idea before, but how about an unstable-optimisations feature flag to swap implementations for this stuff around. It could be Box for most users and FixedString for crazy people.

This could work and would save you the headache of maintaining a fork. That work could be made in a separate PR, as the sizable savings already incurred from switching to Box are worth merging on their own merit.

@GnomedDev GnomedDev force-pushed the fixedstring branch 2 times, most recently from 99445b6 to 947cb6f Compare December 8, 2023 16:53
@github-actions github-actions bot added the ci Related to the continuous integration. label Dec 8, 2023
GnomedDev added a commit that referenced this pull request Mar 11, 2024
…l models (#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.
GnomedDev added a commit that referenced this pull request Mar 11, 2024
…l models (#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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Mar 13, 2024
…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.
GnomedDev added a commit that referenced this pull request Mar 13, 2024
…l models (#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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Mar 19, 2024
…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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Mar 19, 2024
…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.
GnomedDev added a commit that referenced this pull request Mar 21, 2024
…l models (#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.
GnomedDev added a commit that referenced this pull request Mar 25, 2024
…l models (#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.
GnomedDev added a commit that referenced this pull request Mar 29, 2024
…l models (#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.
GnomedDev added a commit that referenced this pull request Mar 31, 2024
…l models (#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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Mar 31, 2024
…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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Apr 1, 2024
…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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request May 14, 2024
…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.
GnomedDev added a commit that referenced this pull request May 14, 2024
…l models (#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.
GnomedDev added a commit that referenced this pull request May 23, 2024
…l models (#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.
GnomedDev added a commit that referenced this pull request May 28, 2024
…l models (#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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Jun 9, 2024
…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.
GnomedDev added a commit that referenced this pull request Jun 22, 2024
…l models (#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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Jun 22, 2024
…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.
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jul 29, 2024
…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.
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jul 30, 2024
…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.
GnomedDev added a commit that referenced this pull request Aug 16, 2024
…l models (#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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Oct 7, 2024
…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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Oct 20, 2024
…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.
GnomedDev added a commit that referenced this pull request Oct 20, 2024
…l models (#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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Nov 11, 2024
…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.
GnomedDev added a commit that referenced this pull request Nov 13, 2024
…l models (#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.
GnomedDev added a commit to GnomedDev/serenity that referenced this pull request Nov 15, 2024
…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.
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Dec 8, 2024
…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.
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jan 16, 2025
…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.
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 builder Related to the `builder` module. cache Related to the `cache`-feature. client Related to the `client` module. enhancement An improvement to Serenity. examples Related to Serenity's examples. framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate gateway Related to the `gateway` module. model Related to the `model` module. utils Related to the `utils` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants