Fixed memory_pool::min_block_size calculation

[jwdevel]

Previously, memory_pool::min_block_size() would not compute true node
size in the same way that the individual free list implementations did.
For instance, ordered_free_memory_list uses 2*node_size as debug fence
memory, which memory_pool was disregarding. This could result in a
failure at runtime, if the block was too small for a full node,
including debug fences.

Now, each free_list impl exposes "actual_node_size(n)", which produces
the true required size, given the requested node size. This function is
the same as what's used in the real internal size computations.

Also hoisted fence_size() into constexpr functions in each free_list
implementation, as well as added an "adjusted_node_size" function, which
is mainly for readability, to avoid having "x > y ? x : y" repeated.

Test added to exercise the bug.

Addresses issue #109

[jwdevel]

Note: the test added is pretty minimal; I'm curious if there are other tests that you think would be worthwhile.

[jwdevel]

This is one part I'm not 100% sure of. Is min_node_size meant to be the minimum amount that can be requested (in which case I should not change it), or is it meant to be the minimum you'll actually get, as it is in this update?

[MiguelCompany]

@jwdevel Would you mind adding something similar to this so users could be aware of the existence of the new API?

[jwdevel]

@MiguelCompany Sure — is it purely a case of adding a new #define or is it hooked up to doxygen or something like that? I'm not deeply familiar with this project yet ( :

Also: note that the added public functions are all in the detail:: interface. Not sure if they should be part of the public API...

[MiguelCompany]

@jwdevel I'm just a user of this project that has done some contributions. I did contribute the method you are fixing, and that is why I'm commenting on this PR, as I am highly interested on it working properly. I write this just to say that the final judgement should be from @foonathan.

is it purely a case of adding a new #define or is it hooked up to doxygen or something like that?

I was just thinking on the #define. Let's see what @foonathan says.

Also: note that the added public functions are all in the detail:: interface. Not sure if they should be part of the public API...

🤔 Even though they are in the detail:: interface I think they are exposed on include/foonathan/memory/memory_pool_type.hpp, so I guess that would be the place to add the doxygen for it

Looking at that file, I also see that this PR is missing changes on detail::array_free_memory_list.

[jwdevel]

Looking at that file, I also see that this PR is missing changes on detail::array_free_memory_list.

Eh? That type is just a typedef for ordered_free_memory_list, which is covered:

    using array_free_memory_list = ordered_free_memory_list;

Unless I misunderstand something?

---

That aside, I am now adding tests for array_pool and small_node_pool, and the small_node_pool one is failing.
It also fails on mainline, so not a regression per se, but I'll look into that and post an update to this PR. Not sure what's going on, there.

---

All that aside, yes, I agree would like to hear back from @foonathan about the public API/#define stuff, too ( :

[MiguelCompany]

That type is just a typedef for ordered_free_memory_list

You are right! Sorry for the confusion!

[jwdevel]

Regarding the failure in small_node_pool:
It seems actual_node_size may not be sufficient, in this case. The code looks like:

auto actual_size      = actual_node_size(node_size_);                            // This is 408, in the test
auto total_chunk_size = chunk::memory_offset + actual_size * chunk::max_nodes;   // this is 104072 [ = 32 + 408 * 255 ]

...

auto no_chunks = size / (total_chunk_size + align_buffer);  // This is therefore 0 [ = 408 / (104072 + 0) ]

And therefore it allocates 0 new nodes from the chunk, and fails the assert.

So it seems like instead of actual_node_size, we might want min_chunk_size to be made available from each free list impl?
For the small_free_list, the calculation is somewhat more complex.

[foonathan]

I've started reviewing this PR, but I found many API inconsistencies and potential bugs (in my code, not yours). Thank you for doing that, but I'll think I need to refactor a bit of the free list logic in general. For starters, I'm not sure the debug fence size handling of the small node list is correct at all with regard to alignment, and I have absolutely no idea why .allocate(n) requires bytes and not the number of nodes etc.

I'll see what I can come up with.

[jwdevel]

@foonathan Do you want me to create an issue for this overall set of problems? I could add some (failing) tests, if you like.
No worries if you just want to track it yourself though.

[foonathan]

I think I've already fixed it?

[jwdevel]

Oh, my mistake — sorry, I had not seen the pushes you made.
Thanks ( :