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

Implement freelist class #783

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

veronikaro
Copy link

@veronikaro veronikaro commented Oct 14, 2024

Hey @gavv,

the PR is not ready yet (obvious lack of tests, still some work on the code) but I wanted to double check whether this goes in the direction you envisioned.
Especially regarding the very limited functionality of the FreeList.
Another thing: Right now I use the default memory order coming from the Atomic templates (sequentially consistent ordering). This should work in principle, but deviates from what is suggested in the article you linked.
To my understanding, using the stricter memory ordering might introduce some performance issues under heavy contention.
Since a more fine grained memory order control is possible with the AtomicOps, I was wondering what you think about this.

#734

@github-actions github-actions bot added the work in progress Pull request is still in progress and changing label Oct 14, 2024
Copy link

🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.

@veronikaro veronikaro changed the base branch from master to develop October 14, 2024 21:43
@veronikaro veronikaro marked this pull request as ready for review November 3, 2024 12:03
@github-actions github-actions bot added ready for review Pull request can be reviewed and removed work in progress Pull request is still in progress and changing labels Nov 3, 2024
@gavv gavv added the contribution A pull-request by someone else except maintainers label Nov 22, 2024
@gavv
Copy link
Member

gavv commented Nov 22, 2024

Hi, sorry for late reply, I was ill. Will take a look in upcoming days, thanks for the patch!

@gavv gavv changed the title Issue 734 Implement freelist class Nov 22, 2024
@gavv gavv self-requested a review November 28, 2024 13:54
@gavv
Copy link
Member

gavv commented Dec 19, 2024

Sorry again for delay. The patch looks great overall!

I wanted to double check whether this goes in the direction you envisioned [...] regarding the very limited functionality of the FreeList.

Yes, this is intended: lock-free structures typically support only a limited set of operations. In this case, only two: push_front() (add() in the article) and try_pop_front() (try_get() in the article).

This is perfect for us, because we want to use free list as a cache for a slab pool. Using LIFO is preferred over FIFO here, because recently deallocated objects have a higher chance to of still being in the CPU cache.

Another thing: Right now I use the default memory order coming from the Atomic templates (sequentially consistent ordering). This should work in principle, but deviates from what is suggested in the article you linked. To my understanding, using the stricter memory ordering might introduce some performance issues under heavy contention. Since a more fine grained memory order control is possible with the AtomicOps, I was wondering what you think about this.

Yes, lock-free operations usually have overhead, and using SEQ_CST everywhere only increases it.

Personally, I try to avoid inventing my own lock-free algorithms, and if I have to, I stick with SEQ_CST everywhere, as I don't feel proficient enough in this topic. Hence, core::Atomic is SEQ_CST-only, to keep its usage in the codebase simpler.

However, when we're implementing a well-known algorithm where the correct memory barriers were already carefully designed and reviewed by many eyeballs, using ACQ/REL is beneficial. So in this case, yes, it would be nice to use lower-level core::AtomicOps and replicate barriers from the article, just like we do in MpscQueue (which implements Dmitry Vyukov queue).

With this approach, implementation of containers like MpscQueue and FreeList can be more complicated, but based on a reference code and thus verifiable. And our custom code that uses core::Atomic, MpscQueue, etc, can remain simpler and much easier to verify and review.

Feedback on the code

  1. FreeList can't inherit from List, as it doesn't support most List operations.

  2. free_list_refs is accessed more frequently, so better to keep it the first field, like in the article. Also we could name it just refs, I think.

  3. Let's rename pop_front() to try_pop_front(). The difference is that it can return NULL even if freelist is not empty, if there is contention and we're unlucky.

  4. try_pop_front() should return NULL if list is empty, instead of panic, because the caller has no way to know if the list is empty other than by trying to call try_pop_front().

  5. Let's also keep comments from the article, and add a link to it.

  6. In FreeListImpl we also need unsafe_pop_front() that can be called only when the list is not being used by anyone. In FreeList we don't need to expose it, but we should use it in ~FreeList destructor to release ownership of all objects (like we do in in core::List).

  7. nit: add_knowing_refcount_is_zero => add_knowing_refcount_is_zero_ because it's a private method. Also, constants like SHOULD_BE_ON_FREELIST could be moved to the .cpp file, I think.

@gavv gavv added work in progress Pull request is still in progress and changing and removed ready for review Pull request can be reviewed labels Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution A pull-request by someone else except maintainers work in progress Pull request is still in progress and changing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants