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

Add an overflow push method #58

Merged
merged 5 commits into from
Mar 30, 2024
Merged

Add an overflow push method #58

merged 5 commits into from
Mar 30, 2024

Conversation

notgull
Copy link
Member

@notgull notgull commented Feb 11, 2024

In some cases it is desired to have a "lossy" queue for data. Such as an
event queue where more recent events should be prioritized over older
ones, where infinite storage is impractical. This commit adds a method
called "force_push" which enables this usage.

Bounded queue code is partially derived from the following commit:
crossbeam-rs/crossbeam@bd75c3c

cc smol-rs/async-channel#44

In some cases it is desired to have a "lossy" queue for data. Such as an
event queue where more recent events should be prioritized over older
ones, where infinite storage is impractical. This commit adds a method
called "force_push" which enables this usage.

Bounded queue code is partially derived from the following commit:
crossbeam-rs/crossbeam@bd75c3c

cc smol-rs/async-channel#44

Signed-off-by: John Nunley <[email protected]>
@notgull notgull requested a review from fogti February 11, 2024 19:17
Signed-off-by: John Nunley <[email protected]>
@notgull notgull requested a review from zeenix March 2, 2024 03:36
Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like @taiki-e to also have a look at this as they're more familiar with this repo.

src/bounded.rs Show resolved Hide resolved
@notgull notgull requested review from fogti and taiki-e March 23, 2024 16:03
// Swap out the old value.
// SAFETY: We know this is initialized, since it's covered by the current queue.
let old = unsafe {
slot.value
Copy link
Member

Choose a reason for hiding this comment

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

I'll ask for a small clarification: why is the slot the correct one to put the new value inside? (in contrast to a slot derived from the new_tail)

Copy link
Member Author

Choose a reason for hiding this comment

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

slot is the old tail, which means it's the last unfilled slot.

@@ -132,13 +201,9 @@ impl<T> Bounded<T> {
}
} else if stamp.wrapping_add(self.one_lap) == tail + 1 {
Copy link
Member

Choose a reason for hiding this comment

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

why does this work with tail + 1 and not use something like what's done for new_tail?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done in order to tell if the stamp has advanced to where the tail would logically be, as opposed to where the tail actually is.

I'm not 100% clear on the logic here; this was written long before I joined smol-rs, and I think it was originally part of crossbeam.

Copy link
Member

Choose a reason for hiding this comment

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

ok, it mght be a good idea to maybe investigate and properly document this sometime (not even necessarily inside of the code, a separate tex document with diagrams of this might be a good idea). Maybe I find time to do that (but unlikely in the next few months because I need to time to write a Bachelor thesis).

src/single.rs Outdated Show resolved Hide resolved
Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
@notgull notgull merged commit c407467 into master Mar 30, 2024
8 checks passed
@notgull notgull deleted the notgull/overflow branch March 30, 2024 23:38
@notgull notgull mentioned this pull request Apr 5, 2024
@@ -74,6 +74,32 @@ fn close() {
assert_eq!(q.pop(), Err(PopError::Closed));
}

#[test]
fn force_push() {
let q = ConcurrentQueue::<i32>::bounded(5);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a test for bounded queue, but the test file is tests/unbounded.rs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #68

Copy link
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm concerned that the Loom test and concurrent access test (like this) were not added.

self.state.fetch_and(!LOCKED, Ordering::Release);

// If the value was pushed, initialize it and return it.
let prev_value = if prev & PUSHED == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think by processing this before the fetch_and, the cost of moving the previous value when PUSHED == 0 can be avoided. (The compiler may be able to handle it well in release mode, but at least it would be useful in debug mode when the element is costly to move, such as large arrays.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #71

@fogti
Copy link
Member

fogti commented Apr 22, 2024

Oh sorry, I'll keep that in mind to audit test suite updates more closely in the future for completeness and such.

@notgull
Copy link
Member Author

notgull commented Apr 23, 2024

More tests are added in #70

notgull added a commit that referenced this pull request Apr 25, 2024
notgull added a commit that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants