Add FixedQueue: A Fixed Size Queue Implementation #126204

Alex-Velez · 2024-06-09T19:54:04Z

A new data structure, FixedQueue, which is a fixed-size queue implemented using a static ring buffer.

Both push and pop operations should be constant time.
Elements should be stored contiguously in memory.
Unit tests should cover all methods and functionality.

I recognize that Rust already has VecDeque, but it's growable and has a more general implementation. Please correct me if I'm wrong, but I believe this might offer a more efficient solution for scenarios where a fixed-size queue is sufficient.

I want to learn and am open to any suggestions or critiques.

Example Usage:

use std::collections::FixedQueue;

let mut fque = FixedQueue::<i32, 3>::new();
fque.push(1);
fque.push(2);
fque.push(3);
assert_eq!(fque.pop(), Some(1));
assert_eq!(fque.pop(), Some(2));
fque.push(4);
assert_eq!(fque.to_vec(), vec![3, 4]);

A new data structure, `FixedQueue`, which is a fixed-size queue implemented using a static ring buffer. * Both push and pop operations should be constant time. * Elements should be stored contiguously in memory. * Unit tests should cover all methods and functionality. I recognize that Rust already has `VecDeque`, but it's growable and has a more general implementation. Please correct me if I'm wrong, but I believe this might offer a more efficient solution for scenarios where a fixed-size queue is sufficient. I want to learn and am open to any suggestions or critiques. Example Usage: ```rust use std::collections::FixedQueue; let mut fque = FixedQueue::<i32, 3>::new(); fque.push(1); fque.push(2); fque.push(3); assert_eq!(fque.pop(), Some(1)); assert_eq!(fque.pop(), Some(2)); fque.push(4); assert_eq!(fque.to_vec(), vec![3, 4]); ```

rustbot · 2024-06-09T19:54:12Z

r? @Nilstrieb

rustbot has assigned @Nilstrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

mingw-check-tidy formatting errors fix

Fixing file formatting errors

Fixing file fomatting errors

insert tests

moved fixed queue into seperate folder

allow fixed queue to be a module

add unstable feature

include vec and string

add feature

unstable tags

including docs

add symbol

sort by alphabetical order

feature gate test

std to alloc (perhaps wrong place)

use feature in example

rust-log-analyzer · 2024-06-10T16:46:18Z

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#16 exporting to docker image format
#16 sending tarball 29.3s done
#16 DONE 34.4s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
normalized stderr:
error[E0433]: failed to resolve: use of undeclared type `FixedQueue`
##[error]  --> $DIR/feature-gate-fixed-queue.rs:6:18
   |
LL |     let _queue = FixedQueue::<i32, 3>::new();
   |                  ^^^^^^^^^^ use of undeclared type `FixedQueue`
error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0433`.




The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-fixed-queue/feature-gate-fixed-queue.stderr
To only update this specific test, also pass `--test-args feature-gates/feature-gate-fixed-queue.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/feature-gates/feature-gate-fixed-queue.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-fixed-queue" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-fixed-queue/auxiliary"
--- stderr -------------------------------
error[E0433]: failed to resolve: use of undeclared type `FixedQueue`
##[error]  --> /checkout/tests/ui/feature-gates/feature-gate-fixed-queue.rs:6:18
   |
   |
LL |     let _queue = FixedQueue::<i32, 3>::new(); //~ ERROR the type `FixedQueue` is unstable
   |                  ^^^^^^^^^^ use of undeclared type `FixedQueue`
error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0433`.
------------------------------------------

workingjubilee

I see you like AI, and also have submitted some unusual Rust code that is... unidiomatic, especially considering the surrounding standard library idioms, as I hope my review comments illuminate. It more closely resembles C or JavaScript code I have read before. Before I kick this over to T-libs-api, I feel duty-bound to simply ask: Did you use an LLM to generate this code?

workingjubilee · 2024-06-10T19:28:00Z

library/alloc/src/collections/fixed_queue/mod.rs

+        for i in 0..N {
+            drop(self.buffer[i].take());
+        }


if N is large, this attempts to run drop N times, even if the amount of elements actually in the queue is 1 or even 0, which seems...

...computationally wasteful, even with branch predictors being real. It could thrash the entire cache, for instance.

workingjubilee · 2024-06-10T19:28:56Z

library/alloc/src/collections/fixed_queue/mod.rs

+/// [`pop`]: FixedQueue::pop
+#[derive(Debug)]
+#[unstable(feature = "fixed_queue", issue = "126204")]
+pub struct FixedQueue<T, const N: usize> {


Have you considered an implementation where N is not a constant, but is runtime-determined, yet immutable?

workingjubilee · 2024-06-10T19:48:09Z

library/alloc/src/collections/fixed_queue/mod.rs

+    where
+        Option<T>: Copy,
+    {
+        self.buffer = [Some(item); N];


I believe this semantically constructs the entire array as a temporary, then writes it. In the past, this sort of thing has led to unfortunate amounts of memcpy on the stack. I think it would be better to do

Suggested change

self.buffer = [Some(item); N];

for element in self.iter_mut() {

*element = Some(item);

}

That should still optimize appropriately.

workingjubilee · 2024-06-10T19:52:09Z

library/alloc/src/collections/fixed_queue/mod.rs

+    pub fn pop(&mut self) -> Option<T>
+    where
+        Option<T>: Copy,


Some of the most critical API being bounded on Option<T>: Copy, which requires T: Copy, is not really acceptable. It should work without it. This should have tests that confirm it works even if the queue is of Mutex<Vec<T>> or something else painful.

Suggested change

pub fn pop(&mut self) -> Option<T>

where

Option<T>: Copy,

pub fn pop(&mut self) -> Option<T> {

workingjubilee · 2024-06-10T19:53:38Z

compiler/rustc_feature/src/unstable.rs

+    /// New collection
+    (unstable, fixed_queue, "1.77.1", Some(126204)),


We don't use this for library features.

Suggested change

/// New collection

(unstable, fixed_queue, "1.77.1", Some(126204)),

workingjubilee · 2024-06-10T20:25:27Z

library/alloc/src/collections/fixed_queue/mod.rs

+}
+
+#[unstable(feature = "fixed_queue", issue = "126204")]
+impl<T: Copy, const N: usize> From<&[T]> for FixedQueue<T, N> {


Suggested change

impl<T: Copy, const N: usize> From<&[T]> for FixedQueue<T, N> {

impl<T, const N: usize> From<&[T]> for FixedQueue<T, N> {

workingjubilee · 2024-06-10T20:26:13Z

library/alloc/src/collections/fixed_queue/mod.rs

+///
+/// [`push`]: FixedQueue::push
+/// [`pop`]: FixedQueue::pop
+#[derive(Debug)]


This should probably not derive Debug, but rather have an implementation that guarantees that the elements are printed from head to tail.

workingjubilee · 2024-06-10T20:27:02Z

library/alloc/src/collections/fixed_queue/mod.rs

+        // create temporary array to store the results
+        let mut temp = Vec::with_capacity(end - start);


...No.

This isn't an array, this is a Vec.

This is not an acceptable implementation.

workingjubilee · 2024-06-10T20:27:28Z

library/alloc/src/collections/fixed_queue/mod.rs

+        // Return a slice from the temporary array
+        // SAFETY: This is safe because temp will live long enough within this function call.
+        let result = unsafe { slice::from_raw_parts(temp.as_ptr(), temp.len()) };
+        mem::forget(temp);
+        result


This creates a memory leak for performing simple indexing. This should successfully run to completion if you use fixed_queue[0..N] on a FixedQueue of 65535 elements a total of 2147483647 times in a program, without increasing memory usage.

workingjubilee · 2024-06-10T20:33:54Z

library/alloc/src/collections/fixed_queue/mod.rs

+#[derive(Debug)]
+#[unstable(feature = "fixed_queue", issue = "126204")]
+pub struct FixedQueue<T, const N: usize> {
+    buffer: [Option<T>; N],


This representation is... bad. It requires the memory leak in Index<Range<usize>> to yield [T]. Please come back with a version that uses MaybeUninit instead:

Suggested change

buffer: [Option<T>; N],

buffer: [MaybeUninit<T>; N],

This would, of course, require much more unsafe code in order to implement correctly, with which an overconfident yes-man cannot help. I strongly advise not even trying to ask ChatGPT, Codex, Copilot, or any other code-generation tool that you may or may not be using about unsafe code. They will give you wrong answers, or bad ones.

workingjubilee · 2024-06-10T20:39:45Z

I strongly recommend you actually learn to run the standard library build on a local machine... or even a remote one you have full shell access to... if you want to contribute significant data structures to rust-lang/rust, instead of using the GitHub web interface for all of your edits. Apologies if you already are or are having difficulties. I can help on the Rust Zulip if you wish to try.

workingjubilee · 2024-06-10T20:49:43Z

I recognize that Rust already has VecDeque, but it's growable and has a more general implementation. Please correct me if I'm wrong, but I believe this might offer a more efficient solution for scenarios where a fixed-size queue is sufficient.

"A more efficient solution for scenarios where..."

This particular clause is borderline irrelevant for deciding whether something is added to the standard library. There are, when you consider significant implementation differences, millions of data structures that are more efficient or optimal in some specific case. The standard library is not necessarily aiming to have millions of data structures. The path to adding API is expanded on in greater length here: https://std-dev-guide.rust-lang.org/development/feature-lifecycle.html

There is probably some appetite for accepting an ArrayVec implementation into the standard library, but I don't see why we would accept a FixedQueue with ArrayVec-like implementation first.

scottmcm · 2024-06-14T18:39:02Z

A new data structure

A whole new data structure is a big enough thing that you absolutely need at least an API change proposal for it. It might even end up needing an RFC. As such, I'm actually just going to close this -- a new data structure is a much bigger topic than just a new inherent method here or there.

In particular, there's people working on things like rust-lang/wg-allocators#93 with the goal that VecDeque can be the fixed-size double-ended queue just by changing the allocator, rather than adding another type.

I would suggest that you turn your code into a crate instead of submitting it here. Then people can use it immediately, whereas if you submit it to core people can't use it for a minimum of 11 weeks -- and more likely people won't be able to use it for a year or more. When something doesn't need privileged access to internal stuff, a crate is more useful.

rustbot assigned Noratrieb Jun 9, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 9, 2024

This comment has been minimized.

Sign in to view

Update fixed_queue.rs

1fab98c

mingw-check-tidy formatting errors fix

This comment has been minimized.

Sign in to view

Alex-Velez added 5 commits June 9, 2024 15:30

Create mod.rs

f5f4eae

Fixing file formatting errors

Create tests.rs

6fa3e09

Fixing file fomatting errors

Update mod.rs

f389904

insert tests

Delete library/alloc/src/collections/fixed_queue.rs

d6b179b

moved fixed queue into seperate folder

Update mod.rs

21b4b61

allow fixed queue to be a module

This comment has been minimized.

Sign in to view

Alex-Velez added 2 commits June 9, 2024 19:33

Update mod.rs

11907dc

add unstable feature

Update tests.rs

107c392

include vec and string

This comment has been minimized.

Sign in to view

Alex-Velez added 2 commits June 9, 2024 19:39

Update lib.rs

3274628

add feature

Update mod.rs

a7fa2b9

This comment has been minimized.

Sign in to view

Update unstable.rs

1cf312e

This comment has been minimized.

Sign in to view

Update mod.rs

579c9c2

unstable tags

This comment has been minimized.

Sign in to view

Update mod.rs

fcda44f

including docs

This comment has been minimized.

Sign in to view

Update symbol.rs

f03b7b4

add symbol

This comment has been minimized.

Sign in to view

Update unstable.rs

8f02970

sort by alphabetical order

This comment has been minimized.

Sign in to view

Create feature-gate-fixed-queue.rs

a30dc93

feature gate test

This comment has been minimized.

Sign in to view

Update mod.rs

e6a7ebd

std to alloc (perhaps wrong place)

This comment has been minimized.

Sign in to view

Update mod.rs

22b4774

use feature in example

workingjubilee added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 10, 2024

workingjubilee requested changes Jun 10, 2024

View reviewed changes

scottmcm closed this Jun 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FixedQueue: A Fixed Size Queue Implementation #126204

Add FixedQueue: A Fixed Size Queue Implementation #126204

Alex-Velez commented Jun 9, 2024

rustbot commented Jun 9, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rust-log-analyzer commented Jun 10, 2024

workingjubilee left a comment

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024 •

edited

Loading

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024

workingjubilee Jun 10, 2024

workingjubilee commented Jun 10, 2024 •

edited

Loading

workingjubilee commented Jun 10, 2024 •

edited

Loading

scottmcm commented Jun 14, 2024

-        self.buffer = [Some(item); N];
+        for element in self.iter_mut() {
+            *element = Some(item);
+        }

		/// New collection
		(unstable, fixed_queue, "1.77.1", Some(126204)),

	impl<T: Copy, const N: usize> From<&[T]> for FixedQueue<T, N> {
	impl<T, const N: usize> From<&[T]> for FixedQueue<T, N> {

		// create temporary array to store the results
		let mut temp = Vec::with_capacity(end - start);

Add FixedQueue: A Fixed Size Queue Implementation #126204

Add FixedQueue: A Fixed Size Queue Implementation #126204

Conversation

Alex-Velez commented Jun 9, 2024

rustbot commented Jun 9, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rust-log-analyzer commented Jun 10, 2024

workingjubilee left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Jun 10, 2024 • 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

workingjubilee commented Jun 10, 2024 • edited Loading

workingjubilee commented Jun 10, 2024 • edited Loading

scottmcm commented Jun 14, 2024

workingjubilee Jun 10, 2024 •

edited

Loading

workingjubilee commented Jun 10, 2024 •

edited

Loading

workingjubilee commented Jun 10, 2024 •

edited

Loading