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

Archetype::grow leaks previously allocated memory #79

Closed
ghost opened this issue Sep 23, 2020 · 3 comments · Fixed by #80
Closed

Archetype::grow leaks previously allocated memory #79

ghost opened this issue Sep 23, 2020 · 3 comments · Fixed by #80

Comments

@ghost
Copy link

ghost commented Sep 23, 2020

With this test case on master:

#[test]
fn spawn_batch_unexact() {
    struct Unhint<I>(I);
    impl<I: Iterator> Iterator for Unhint<I> {
        type Item = I::Item;

        fn next(&mut self) -> Option<Self::Item> {
            self.0.next()
        }

        fn size_hint(&self) -> (usize, Option<usize>) {
            (0, None)
        }
    }

    let mut world = World::new();
    world.spawn_batch(Unhint((0..100).map(|x| (x, "abc"))));
    let entities = world
        .query::<&i32>()
        .iter()
        .map(|(_, &x)| x)
        .collect::<Vec<_>>();
    assert_eq!(entities.len(), 100);
}

miri reports:

alloc4621966 (Rust heap, size: 1280, align: 8) {
    0x000 │ ╾a4468557[<11121328>]─╼ 03 00 00 00 00 00 00 00 │ ╾──────╼........
    // ... repeats ...
    0x400 │ 00 00 00 00 01 00 00 00 02 00 00 00 03 00 00 00 │ ................
    0x410 │ 04 00 00 00 05 00 00 00 06 00 00 00 07 00 00 00 │ ................
    // ... repeats ...
    0x4f0 │ 3c 00 00 00 3d 00 00 00 3e 00 00 00 3f 00 00 00 │ <...=...>...?...
}
alloc4468557 (global, size: 3, align: 1) {
    61 62 63                                        │ abc
}
alloc4622641 (global, size: 3, align: 1) {
    61 62 63                                        │ abc
}
// ... repeats ...

cargo-valgrind reports:

       Error Leaked 1.2 kiB
        Info at malloc (vg_replace_malloc.c:307)
             at alloc::alloc::alloc (alloc.rs:80)
             at hecs::archetype::Archetype::grow (archetype.rs:195)
             at hecs::archetype::Archetype::allocate (archetype.rs:159)
             at <hecs::world::SpawnBatchIter<I> as core::iter::traits::iterator::Iterator>::next (world.rs:763)
             at <&mut I as core::iter::traits::iterator::Iterator>::next (iterator.rs:3283)
             at <hecs::world::SpawnBatchIter<I> as core::ops::drop::Drop>::drop (world.rs:748)
             at core::ptr::drop_in_place (mod.rs:184)
             at tests::spawn_batch_unexact (tests.rs:355)
             at tests::spawn_batch_unexact::{{closure}} (tests.rs:340)
             at core::ops::function::FnOnce::call_once (function.rs:232)
             at call_once<(),FnOnce<()>> (boxed.rs:1076)
             at call_once<(),alloc::boxed::Box<FnOnce<()>>> (panic.rs:318)
             at do_call<std::panic::AssertUnwindSafe<alloc::boxed::Box<FnOnce<()>>>,()> (panicking.rs:297)
             at try<(),std::panic::AssertUnwindSafe<alloc::boxed::Box<FnOnce<()>>>> (panicking.rs:274)
             at catch_unwind<std::panic::AssertUnwindSafe<alloc::boxed::Box<FnOnce<()>>>,()> (panic.rs:394)
             at run_test_in_process (lib.rs:541)
             at test::run_test::run_test_inner::{{closure}} (lib.rs:450)
       Error Leaked 2.0 MiB
        Info at malloc (vg_replace_malloc.c:307)
             at alloc::alloc::alloc (alloc.rs:80)
             at hecs::archetype::Archetype::grow (archetype.rs:195)
             at hecs::archetype::Archetype::allocate (archetype.rs:159)
             at hecs::world::World::spawn (world.rs:98)
             at tests::spawn_many (tests.rs:248)
             at tests::spawn_many::{{closure}} (tests.rs:244)
             at core::ops::function::FnOnce::call_once (function.rs:232)
             at call_once<(),FnOnce<()>> (boxed.rs:1076)
             at call_once<(),alloc::boxed::Box<FnOnce<()>>> (panic.rs:318)
             at do_call<std::panic::AssertUnwindSafe<alloc::boxed::Box<FnOnce<()>>>,()> (panicking.rs:297)
             at try<(),std::panic::AssertUnwindSafe<alloc::boxed::Box<FnOnce<()>>>> (panicking.rs:274)
             at catch_unwind<std::panic::AssertUnwindSafe<alloc::boxed::Box<FnOnce<()>>>,()> (panic.rs:394)
             at run_test_in_process (lib.rs:541)
             at test::run_test::run_test_inner::{{closure}} (lib.rs:450)
             at std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:130)
             at {{closure}}<closure-0,()> (mod.rs:475)
             at call_once<(),closure-0> (panic.rs:318)
             at do_call<std::panic::AssertUnwindSafe<closure-0>,()> (panicking.rs:297)
             at try<(),std::panic::AssertUnwindSafe<closure-0>> (panicking.rs:274)
             at catch_unwind<std::panic::AssertUnwindSafe<closure-0>,()> (panic.rs:394)
             at {{closure}}<closure-0,()> (mod.rs:474)
             at core::ops::function::FnOnce::call_once{{vtable-shim}} (function.rs:232)
             at call_once<(),FnOnce<()>> (boxed.rs:1076)
             at call_once<(),alloc::boxed::Box<FnOnce<()>>> (boxed.rs:1076)
             at std::sys::unix::thread::Thread::new::thread_start (thread.rs:87)
     Summary Leaked 2.0 MiB total

Note that valgrind also reported leaked memory for spawn_many, which is ignored for miri, but it seems likely that both leaks may have the same underlying reason.

@ghost
Copy link
Author

ghost commented Sep 23, 2020

Well, seems like this has a simple cause: the old allocation isn't freed in Archetype::grow! self.data = UnsafeCell::new(new_data); merely dropped the pointer, the allocation isn't freed.

@ghost ghost changed the title spawn_batch leaks memory when called with an iterator that does not have an exact size_hint Archetype::grow leaks previously allocated memory Sep 23, 2020
@Ralith
Copy link
Owner

Ralith commented Sep 23, 2020

Whoops, thanks for catching that! Will push a fix tonight.

@Ralith
Copy link
Owner

Ralith commented Sep 24, 2020

Fix released in 0.2.15.

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 a pull request may close this issue.

1 participant