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

Skip slots v2 #81

Merged
merged 19 commits into from
Apr 6, 2024
Merged

Skip slots v2 #81

merged 19 commits into from
Apr 6, 2024

Conversation

tukan
Copy link
Contributor

@tukan tukan commented May 15, 2023

No description provided.

@tukan tukan closed this May 15, 2023
@tukan tukan reopened this May 15, 2023
@tukan tukan changed the title Skip slots v2 [WIP] Skip slots v2 May 15, 2023
@tukan tukan force-pushed the feature/skip-slots-2 branch from faddeb7 to ad96a30 Compare May 15, 2023 17:45
@hawkw
Copy link
Owner

hawkw commented May 15, 2023

hi @tukan, what's the relationship between this PR and your previous #80 ? are you still planning to work on #80 as well?

@tukan
Copy link
Contributor Author

tukan commented May 15, 2023

Hi @hawkw. I created this PR following your suggestions in #80 (to have both implementations available at the same time). If you prefer the approach from this PR, I can merge and stash/rebase all changes back into #80.

Also, I rented a c6i.2xlarge EC2 instance to run benches, here are criterion reports:
benches.tgz

@tukan tukan changed the title [WIP] Skip slots v2 Skip slots v2 May 15, 2023
Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Overall, I definitely prefer this approach over the one proposed in #80. I had a handful of minor suggestions, most of which are just stylistic --- feel free to ignore most of them.

I do think we should ensure the new slow loom models are added to the list of slow models that run in separate CI jobs, so that we can run those tests on CI in the future.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/mpsc/tests/mpsc_blocking.rs Show resolved Hide resolved
src/mpsc/tests/mpsc_async.rs Show resolved Hide resolved
src/mpsc/tests/mpsc_async.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@tukan tukan marked this pull request as draft May 18, 2023 13:54
@tukan tukan force-pushed the feature/skip-slots-2 branch 3 times, most recently from b297517 to 885f4cc Compare May 18, 2023 14:40
@tukan tukan marked this pull request as ready for review May 18, 2023 14:48
@tukan tukan requested a review from hawkw May 18, 2023 15:08
@tukan
Copy link
Contributor Author

tukan commented Oct 19, 2023

Hello @hawkw,

I hope you're well. I wanted to kindly bring to your attention this PR. Could you please finish reviewing it when you have a moment. Thank you.

@hawkw
Copy link
Owner

hawkw commented Apr 6, 2024

Hi, I'm sorry, I must have missed this earlier! I'd love to get this PR merged soon and I'll give it another review shortly.

Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Okay, this looks good to me! I'm going to go ahead and merge it now. Thanks @tukan for the fix!

Comment on lines +466 to +484
#[inline]
fn check_has_reader(state: usize) -> bool {
state & HAS_READER == HAS_READER
}

#[inline]
fn set_has_reader(state: usize) -> usize {
state | HAS_READER
}

#[inline]
fn clear_has_reader(state: usize) -> usize {
state & !HAS_READER
}

#[inline]
fn wrapping_add(a: usize, b: usize) -> usize {
(a + b) & MAX_CAPACITY
}
Copy link
Owner

Choose a reason for hiding this comment

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

There are now enough operations on state that I wonder if it's worth turning it into a newtype...but, we can do that in a separate PR.

@hawkw hawkw merged commit a72a286 into hawkw:main Apr 6, 2024
hawkw added a commit that referenced this pull request Apr 6, 2024
## v0.1.5 (2024-04-06)

#### Features

* **mpsc:**  add `len`, `capacity`, and `remaining` methods to mpsc (#72) ([00213c1](00213c1), closes [#71](#71))

#### Bug Fixes

*   unused import with `alloc` enabled ([ac1eafc](ac1eafc))
*   skip slots with active reading `Ref`s in `push_ref` (#81) ([a72a286](a72a286), closes [#83](#83), [#80](#80))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants