Skip to content

fix: reset pointer to the end of chunk not start - #315

Open
my4ng wants to merge 3 commits into
fitzgen:mainfrom
my4ng:try_with
Open

fix: reset pointer to the end of chunk not start#315
my4ng wants to merge 3 commits into
fitzgen:mainfrom
my4ng:try_with

Conversation

@my4ng

@my4ng my4ng commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

Fix #314.

In the case where (try_)alloc_try_with has an Err result and it was the last allocation with a different chunk than the rewind one, the current_ptr was erroneously set to the start of the chunk rather than the end, which implies that the chunk has no more capacity left for future allocation, since we are bumping upwards. This causes the allocation doubling since it keeps getting new chunks.

I suspect this might be from when the bump direction was changed.

@fitzgen fitzgen left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks! Can you add a regression test to tests/all/try_alloc_try_with.rs?

@fitzgen fitzgen left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks! One last thing below

Comment thread tests/all/tests.rs Outdated
Comment on lines +240 to +263
let bump = Bump::new();
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());

for _ in 0..36 {
bump.alloc_try_with(|| Err::<[u8; 128], _>(())).unwrap_err();
}

// only ONE new chunk should have been allocated
assert_eq!(bump.allocated_bytes(), 974656);

let bump = Bump::new();
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());

for _ in 0..36 {
bump.try_alloc_try_with(|| Err::<[u8; 128], _>(()))
.unwrap_err();
}

// only ONE new chunk should have been allocated
assert_eq!(bump.allocated_bytes(), 974656);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can this test be simplified into the following?

Suggested change
let bump = Bump::new();
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
for _ in 0..36 {
bump.alloc_try_with(|| Err::<[u8; 128], _>(())).unwrap_err();
}
// only ONE new chunk should have been allocated
assert_eq!(bump.allocated_bytes(), 974656);
let bump = Bump::new();
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
bump.alloc_layout(Layout::from_size_align(139040, 128).unwrap());
for _ in 0..36 {
bump.try_alloc_try_with(|| Err::<[u8; 128], _>(()))
.unwrap_err();
}
// only ONE new chunk should have been allocated
assert_eq!(bump.allocated_bytes(), 974656);
let mut bump = Bump::with_capacity(100);
assert_eq!(bump.iter_allocated_chunks().count(), 1);
type R = Result<[u8; 99], ()>;
assert_eq!(mem::size_of::<R>(), 100);
for _ in 0..10 {
bump.alloc_try_with(|| R::Err(())).unwrap_err();
}
assert_eq!(bump.iter_allocated_chunks().count(), 1);
bump.alloc_try_with(|| R::Ok(Default::default())).unwrap();
assert_eq!(bump.iter_allocated_chunks().count(), 1);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the suggested change doesn't fail when the fix is reverted, however I have minimised the test case and taken some of the ideas here.

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.

Extreme allocation growth

2 participants