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

Memory Leak #160

Open
ManeraKai opened this issue Oct 31, 2024 · 17 comments
Open

Memory Leak #160

ManeraKai opened this issue Oct 31, 2024 · 17 comments

Comments

@ManeraKai
Copy link
Contributor

ManeraKai commented Oct 31, 2024

I simplified it as much as possible. Here's my Cargo.toml

[package]
name = "geos-geometry"
version = "0.1.0"
edition = "2021"

[dependencies]
geos = "9.0.0"
rocket = "0.5.1"

main.rs:

#[macro_use]
extern crate rocket;

#[get("/")]
fn index() -> &'static str {
    let _: Vec<geos::Geometry> = (0..2_000_000)
        .map(|_| geos::Geometry::create_empty_point().unwrap())
        .collect();

    "finished"
}

#[launch]
fn rocket() -> _ {
    rocket::build().mount("/", routes![index])
}

GEOS version: 3.12.1-CAPI-1.18.1

Every time I request curl http://127.0.0.1:8000/, I get a +3GB spike in memory. It never frees it. It continues allocating more and more until my OS runs out of memory and crashes. I'm on Ubuntu 24.04.

If you think this is related to rocket, I don't think it is. Here's a code where I only used Vec<i64>:

#[macro_use]
extern crate rocket;

#[get("/")]
fn index() -> &'static str {
    let n: i64 = 1_000_000_000;
    let mut array: Vec<i64> = Vec::with_capacity(n as usize);
    for i in 0..n {
        array.push(i);
    }
    "finished"
}

#[launch]
fn rocket() -> _ {
    rocket::build().mount("/", routes![index])
}

This code spikes memory to about 7GB, but it instantly frees after the request is completed.

Here's the geos-sys version:

use geos::sys::{
    GEOSGeom_createEmptyPoint_r, GEOSGeom_destroy_r, GEOSGeom_t, GEOS_finish_r, GEOS_init_r,
};

#[macro_use]
extern crate rocket;

#[get("/")]
fn index() -> &'static str {
    let ctx = unsafe { GEOS_init_r() };

    let n = 10_000_000;
    let mut geometries: Vec<*mut GEOSGeom_t> = Vec::with_capacity(n);

    for _ in 0..n {
        let geom;
        unsafe {
            geom = GEOSGeom_createEmptyPoint_r(ctx);
        }
        geometries.push(geom);
    }

    for geom in geometries {
        unsafe {
            GEOSGeom_destroy_r(ctx, geom);
        }
    }

    unsafe {
        GEOS_finish_r(ctx);
    }

    "finished"
}

#[launch]
fn rocket() -> _ {
    rocket::build().mount("/", routes![index])
}

Interesting, but why geos::Geometry::create_empty_point() creates a separate context for each new geometry? According to GEOS_init_r(), the context should be passed to other _r functions. I think that's using a lot of memory.
In this geos-sys code, I was able to create 10 million points and only take up 1GB. The memory usage spiked from 1GB, 2GB, 3GB, and then it stayed on 3GB.

@lnicola
Copy link
Member

lnicola commented Oct 31, 2024

I don't think it's a leak, it's just surprising memory allocator behavior in a threaded process. Try (on Linux) with:

[dependencies]
tikv-jemalloc-ctl = { version = "0.6.0", features = ["stats", "use_std"] }
tikv-jemallocator = "0.6.0"
use tikv_jemalloc_ctl::{epoch, stats};
use tikv_jemallocator::Jemalloc;
#[global_allocator]
static GLOBAL: Jemalloc = Jemalloc;

#[get("/")]
fn index() -> &'static str {
    {
        let _: Vec<geos::Geometry> = (0..2_000_000)
            .map(|_| geos::Geometry::create_empty_point().unwrap())
            .collect();
    }

    epoch::advance().unwrap();

    let allocated = stats::allocated::read().unwrap();
    let resident = stats::resident::read().unwrap();
    println!("{} bytes allocated/{} bytes resident", allocated, resident);

    "finished"
}

I get:

1186112 bytes allocated/102133760 bytes resident
1327472 bytes allocated/200015872 bytes resident
1486848 bytes allocated/290623488 bytes resident
1487248 bytes allocated/290557952 bytes resident
1408728 bytes allocated/282210304 bytes resident
1400200 bytes allocated/275214336 bytes resident
1388936 bytes allocated/304959488 bytes resident
1394696 bytes allocated/311848960 bytes resident
1400456 bytes allocated/312528896 bytes resident
1383736 bytes allocated/299057152 bytes resident
1367016 bytes allocated/312176640 bytes resident
1373096 bytes allocated/314605568 bytes resident
1383272 bytes allocated/299708416 bytes resident
1385624 bytes allocated/328593408 bytes resident
1387976 bytes allocated/345174016 bytes resident
1408824 bytes allocated/340930560 bytes resident
1401000 bytes allocated/328921088 bytes resident
1407672 bytes allocated/306122752 bytes resident

While htop actually reports 8 GB RES (but it doesn't go over that).

@ManeraKai
Copy link
Contributor Author

ManeraKai commented Oct 31, 2024

I see.
Why is there a big difference between the allocated report (in megabytes), and htop (in gigabytes)?

@lnicola
Copy link
Member

lnicola commented Oct 31, 2024

I'm not sure. jemalloc reports it as "retained", which should show up as VIRT, but not RES. smaps_rollup reports it as Private_Dirty and Referenced. I couldn't find a way to make jemalloc free it.

And Valgrind says there's no leak.

@lnicola
Copy link
Member

lnicola commented Oct 31, 2024

Ah, so GEOS won't use my custom allocator. Try LD_PRELOAD=/usr/lib64/libjemalloc.so.2. For me, RES stops at 613 MB or so. With MALLOC_CONF="background_thread:true", it even goes back to 102 MB.

@ManeraKai
Copy link
Contributor Author

Thank you!
What about the context handler being created every time a new geometry is created (last paragraph in my issue)?

@ManeraKai
Copy link
Contributor Author

Also, is there a way to make GEOS use tikv_jemallocator? Is that the case for all external bindings?

@lnicola
Copy link
Member

lnicola commented Oct 31, 2024

Yeah, that's not ideal, see also #141.

I don't think the Rust allocator mechanism can handle C libraries. The second option from https://github.com/jemalloc/jemalloc/wiki/getting-started might work, but I'm not sure.

@lnicola
Copy link
Member

lnicola commented Oct 31, 2024

Ah, easy-peasy: tikv-jemallocator = { version = "0.6.0", features = ["unprefixed_malloc_on_supported_platforms"] }.

@ManeraKai
Copy link
Contributor Author

ManeraKai commented Nov 1, 2024

Yeah, that's not ideal, see also #141.

A dumb question, why should Rust be using the Re-entrant API, if we can wrap the regular API with Arc<Mutex>>?

@lnicola
Copy link
Member

lnicola commented Nov 1, 2024

It's a bit weird, TBH. Geometry is Send, so using the global/thread-local contexts directly is probably not an option.

On the other hand, ContextHandle is Send and Sync, which doesn't feel quite right, given that:

Contexts must only be used from a single thread at a time.

Depending on your needs, you could take a look at the geo crate, which saw some interesting developments recently, or at gdal, which hopefully does less shady stuff wrt. Send and Sync.

@ManeraKai
Copy link
Contributor Author

ManeraKai commented Nov 1, 2024

It's a bit weird, TBH. Geometry is Send, so using the global/thread-local contexts directly is probably not an option.
On the other hand, ContextHandle is Send and Sync, which doesn't feel quite right, given that:
Contexts must only be used from a single thread at a time.

I see, weird.

Depending on your needs, you could take a look at the geo crate, which saw some interesting developments recently, or at gdal, which hopefully does less shady stuff wrt. Send and Sync.

I'm already using gdal extensively. I need geos::geometry::Geometry::buffer_with_style though. I might compromise and look again at gdal. geo doesn't have buffer at the moment: georust/geo#641

You still didn't answer my question 😅. If a any non thread-safe function can be wrapped with Arc to share it among threads, and Mutex to mutate it among threads, can't the same logic be applied to C functions as well?

@lnicola
Copy link
Member

lnicola commented Nov 1, 2024

I need geos::geometry::Geometry::buffer_with_style though.

What about BufferEx?

If any non thread-safe function can be wrapped with Arc to share it among threads

It can't. If T: !Send, Arc<T>: !Send.

Mutex to mutate it among threads

A single context and Mutex will yield pretty terrible (i.e. single-threaded or worse) performance.

can't the same logic be applied to C functions as well?

I'm not sure I understand, but it's probably not easy. Ideally, you want as few contexts as possible, to save memory and so on. But on the other hand, you might want to process geometries on multiple threads, and you can't do that within a single context.

PS: Geometry is Send and Sync too 😄.

@ManeraKai
Copy link
Contributor Author

ManeraKai commented Nov 1, 2024

What about BufferEx?

Not present in georust/gdal :')

A single context and Mutex will yield pretty terrible (i.e. single-threaded or worse) performance.

No I don't mean that

I'm not sure I understand, but it's probably not easy. Ideally, you want as few contexts as possible, to save memory and so on. But on the other hand, you might want to process geometries on multiple threads, and you can't do that within a single context.

I'm talking about the use of the normal C API of libgeos that does NOT have context whatsoever. See C API and reentrant C API

@lnicola
Copy link
Member

lnicola commented Nov 1, 2024

Not present in georust/gdal :')

File a PR, an issue, or use gdal-sys?

I'm talking about the use of the normal C API of libgeos that does NOT have context whatsoever.

The "normal" API uses a global context, that's not going to be thread-safe.

@ManeraKai
Copy link
Contributor Author

ManeraKai commented Nov 1, 2024

File a PR, an issue, or use gdal-sys?

I'll try, but I also want geos directly because Geometry is Send and Sync (I can use Rayon) :')

The "normal" API uses a global context, that's not going to be thread-safe.

I see.

It can't. If T: !Send, Arc: !Send.

I see, I need to learn Rust more.

Why are you recommending other tools than this project?

@lnicola
Copy link
Member

lnicola commented Nov 1, 2024

I'll try, but I also want geos directly because Geometry is Send and Sync (I can use Rayon) :')

You can, but are you sure it's correct? I don't know which context GEOS uses for operations that involve two geometries, but you can see how that might be problematic, right? At least you have a different context for each geometry, that's good (helps with thread-safety).

Why are you recommending other tools than this project?

They don't allow as much thread-unsafety, and don't allocate a huge context per geometry?

@ManeraKai
Copy link
Contributor Author

So it's dangerous to use this crate xD.

I mean about the maintenance of this project. Is it maintained?

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

No branches or pull requests

2 participants