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.

Sign up for GitHub

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

Revert PR #53 #63

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

Revert PR #53 #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2822004
Demonstrate the regressions caused by (#53) (commit 8ec1be1) by unit …
ni-vzavalishin Jan 10, 2025
492f9a8
Revert the library code change of (#53), not touching the unit tests
ni-vzavalishin Jan 10, 2025
86b7987
Removed some of the unit tests (those using incorrect partition_sz pa…
ni-vzavalishin Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions include/boost/pool/simple_segregated_storage.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,19 +328,6 @@ void * simple_segregated_storage<SizeType>::try_malloc_n(
void * & start, size_type n, const size_type partition_size)
{
void * iter = nextof(start);
if (n == 1)
{
void * next = nextof(iter);
if (next != static_cast<char *>(iter) + partition_size)
{
start = iter;
return 0;
}
else
{
return iter;
}
}
while (--n != 0)
{
void * next = nextof(iter);
Expand Down
56 changes: 56 additions & 0 deletions test/test_pool_alloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,61 @@ void test_mem_usage()
BOOST_TEST(track_alloc::ok());
}

void test_free_chunk_selection()
{
typedef boost::pool<track_alloc> pool_type;

{
// Expose a regression from the commit 8ec1be1e82ba559744ecfa3c6ec13f71f9c175cc.
// Two checks will fail here.
pool_type pool(sizeof(void *), 3);
void * ptr_0 = pool.ordered_malloc(1);
void * ptr_1 = pool.ordered_malloc(1);
void * ptr_2 = pool.ordered_malloc(1);
// The blocks are expected to be allocated at subsequent locations
BOOST_TEST((char *)ptr_1 - (char *)ptr_0 == sizeof(void *));
BOOST_TEST((char *)ptr_2 - (char *)ptr_1 == sizeof(void *));

pool.ordered_free(ptr_1, 1);

void * ptr_1a = pool.ordered_malloc(1);
// Expected to reallocate the former ptr1 block
// which should be the first and only available block
BOOST_TEST(ptr_1a == ptr_1);

pool.ordered_free(ptr_0, 1);
pool.ordered_free(ptr_1a, 1);
pool.ordered_free(ptr_2, 1);
}

{
// Another way to expose a regression from the commit 8ec1be1e82ba559744ecfa3c6ec13f71f9c175cc.
// This time we preallocate 4 rather than 3 blocks in the pool. In this case
// the location of the ptr_2 block is as expected. The reallocation of ptr_1
// block however still fails the location expectation.
pool_type pool(sizeof(void *), 4);
void * ptr_0 = pool.ordered_malloc(1);
void * ptr_1 = pool.ordered_malloc(1);
void * ptr_2 = pool.ordered_malloc(1);
// The blocks are expected to be allocated at subsequent locations
BOOST_TEST((char *)ptr_1 - (char *)ptr_0 == sizeof(void *));
BOOST_TEST((char *)ptr_2 - (char *)ptr_1 == sizeof(void *));

pool.ordered_free(ptr_1, 1);

void * ptr_1a = pool.ordered_malloc(1);
// Expected to reallocate the former ptr1 block
// which should be the first available block
BOOST_TEST(ptr_1a == ptr_1);

pool.ordered_free(ptr_0, 1);
pool.ordered_free(ptr_1a, 1);
pool.ordered_free(ptr_2, 1);
}

BOOST_TEST(track_alloc::ok());
}

void test_void()
{
typedef boost::pool_allocator<void> void_allocator;
Expand All @@ -313,6 +368,7 @@ int main()
test();
test_alloc();
test_mem_usage();
test_free_chunk_selection();
test_void();

return boost::report_errors();
Expand Down
38 changes: 0 additions & 38 deletions test/test_simple_seg_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,44 +315,6 @@ int main()
BOOST_TEST(nchunks == 2);
}

{
char* const pc = track_allocator::malloc(4 * partition_sz);
test_simp_seg_store tstore;
tstore.add_ordered_block(pc, 4 * partition_sz, partition_sz);

void* pvret = tstore.malloc_n(1, 2 * partition_sz);
BOOST_TEST(pvret == 0);

// There should still be two contiguous
// and one non-contiguous chunk left
std::size_t nchunks = 0;
while(!tstore.empty())
{
tstore.malloc();
++nchunks;
}
BOOST_TEST(nchunks == 4);
}

{
char* const pc = track_allocator::malloc(4 * partition_sz);
test_simp_seg_store tstore;
tstore.add_ordered_block(pc, 4 * partition_sz, partition_sz);

void* pvret = tstore.malloc_n(2, 2 * partition_sz);
BOOST_TEST(pvret == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

We want this to return a nullptr. The existing tests shall not be deleted.

Copy link

Choose a reason for hiding this comment

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

@leimao IMHO this call should fail with an assertion, not by returning a nullptr.

The documentation of the class states:

Simple Segregated Storage is the simplest, and probably the fastest,
memory allocation/deallocation algorithm. It is responsible for
partitioning a memory block into fixed-size chunks: where the block comes from
is determined by the client of the class.

The API design of simple_segregated_storage somewhat implies that one might add blocks with different partition sizes and malloc_n blocks with different partition sizes, but the implementation does not cover any of these cases (and so the available tests - expect the ones added with the debated PR).

Factually, simple_segregated_storage is an implementation detail of pool and the class should be in the detail namespace. Discussing this class in a boost book is also a bad idea, especially when providing examples that do not work correctly.

I consider the added tests as wrong, so IMHO there is no need that these succeed.


// There should still be two contiguous
// and one non-contiguous chunk left
std::size_t nchunks = 0;
while(!tstore.empty())
{
tstore.malloc();
++nchunks;
}
BOOST_TEST(nchunks == 4);
}

{
char* const pc = track_allocator::malloc(12 * partition_sz);
test_simp_seg_store tstore;
Expand Down