Added MemoryLimiter and other classes etc.

[nolan-veed]

Why

For #610

What

• Added MemoryLimiter, LimitedArena, LimitedPool classes.
• Updated and added new tests.

Testing

There are tests.

[github-actions]

🤖 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.

[nolan-veed]

@gavv Please give a quick early review on MemoryLimiter. Does the tracking need to be atomic?

[nolan-veed]

Does the tracking need to be atomic?

I think I've answered my own question. Yes, I suppose, as it's shared across various arenas and pools.

[gavv]

Yep, it should be atomic, and ideally lock free. I guess it would be enough to use core::Atomic or core::AtomicOps.

We likely need to get rid of would_allow because it's usage will be always a race.

Otherwise, the class looks fine!

[gavv]

Oh, also %zu is not portable, please use %lu with a cast to ulong.

[nolan-veed]

Oh, also %zu is not portable, please use %lu with a cast to ulong.

OK. I've used it elsewhere in the code as well.

[nolan-veed]

Yep, it should be atomic, and ideally lock free. I guess it would be enough to use core::Atomic or core::AtomicOps.

See my approach now.

[nolan-veed]

OK. I should have kept this in draft.

I've just added the LimitedArena, LimitedPool, and could use your thoughts on it and a few things. Not yet added tests.

[gavv]

Some notes:

1. Ideally we should also acquire the size of the overhead for every allocated chunk/slot. Arena and pool add headers and guards, and for many allocations this unaccounted overhead will accumulate and may become significant. I guess we need to add methods IArena and IPool to report the overhead, and to take it into account in Limited versions.

2. I think MemoryLimiter::release() should never return false (as well as deallocate). When invariant is failed, it indicates a critical bug, we can panic, but there is no need for error checking because there is anyway to meaningful handling of this error except panic.

3. In CAS loops, please add core::cpu_relax() call.

4. I think we should first deallocate and only then release memory from limiter, to avoid over-allocation.

5. I think we can allow max_bytes = 0 which would mean "no limit". This way we can create all memory limiters unconditionally (even when there is no actual limit), which I think will simplify code.

6. Please log errors when MemoryLimiter::acquire() fails, and include the limit and requested size into the log message. Also, similar to SlabPool, it would be nice to add a name to MemoryLimiter, and include it into the log message too.

7. Please inherit classes from NonCopyable when appreciable.

8. Probably acquiring/releasing zero bytes should be a panic?

9. I think MemoryLimiter destructor should panic if there is unreleased memory. Though, if it would complicate some usages, it's not necessary.

[nolan-veed]

Quick q about... how to approach LimitedPool.

• SlabPool has an EmbeddedCapacity - this means that memory will already be "booked" when it's created.
• IPool has reserve() and allocate(). So, how should the limited version work? I guess we check with MemoryLimiter only on reserve()? So, allocate() and deallocate() just don't bother with the MemoryLimiter?

[gavv]

1. The goal of per-pool limits is to limit number of packets given to one socket/session, so that we can be sure that if one connection flooded, we will not overallocate and other connections won't be affected.

   Hence, I think for per-session LimitedPool it doesn't matter how many packets are booked in embedded array or in slab, since they remain available for other sessions. It only matters how many packets were allocated for this specific session.

   So I think LimitedPool should only acquire and release in allocate/deallocate, and doesn't need to bother about reserve.

2. To control the total amount of memory, we will have root LimitedArena with global limit (max_memory). In the end, all allocations, including objects allocated by pools and pools themselves and their embedded capacity, all will go through this limited arena.

[gavv]

I see some commits, please ping me when it's ready for review :)

[nolan-veed]

I see some commits, please ping me when it's ready for review :)

Yea. Sorry. Small tidy ups are remaining and a few more tests.

[gavv]

I've looked at the implementation and have small suggestion about IPool and IArena interface.

size_of + overhead / object_size + overhead looks a bit too low level, because the caller needs to know that there is "payload" size and "overhead" size. The caller also should assume that the overhead is constant (same for all allocations), which by the way may not be true when arena applies alignment?

Instead of that, I think it would be a bit simpler if the caller will operate with just one "allocation size", which includes payload + overhead.

For IPool, it can be one method, e.g.:

    virtual size_t allocation_size() = 0;

We can remove object_size() since it's not really used and keep only allocation_size() which takes overhead into account.

For IArena, we'd still need 2 methods, but they will both include payload + overhead:

    //! Computes how much bytes will be actually allocated if allocate() is called with given size.
    //! Covers all internal overhead, if any.
    virtual size_t compute_allocated_size(size_t size) = 0;

    //! Returns how much bytes was allocated for given pointer returned by allocate().
    //! Covers all internal overhead, if any.
    //! Returns same value as computed by compute_allocated_size(size).
    virtual size_t allocated_size(void*) = 0;

Feel free to change naming and comments of course.

What do you think?

[gavv]

Thanks for update, I've posted a few more comments.

[nolan-veed]

I've looked at the implementation and have small suggestion about IPool and IArena interface.
...
What do you think?

It does make sense. I did think about something similar to this initially - should have suggested.
I'll go with those names, they are clear.

[nolan-veed]

@gavv
Note: I had to remove this only good use of object_size() as well.

[gavv]

I think we can add size parameter to allocate(). Implementation can then check that requested size fits within slot size. This way we can be sure that any allocation on pool is safe (no matter via new or directly via allocate).

[nolan-veed]

I had to remove num_guard_failures_++; from here, because this method is const. Would you rather have the member variable as mutable?

[gavv]

It makes sense to make it mutable. I'd say it's not part of the logical object state.

[gavv]

Thanks for update, a few more issues remaining.

[gavv]

Thank you! Looks good!

I've created a PR with small refactoring of guard flags in heap arena and slap pool (not directly related to this PR). You can take a look if you wish: #700