-
Notifications
You must be signed in to change notification settings - Fork 0
Updating dggal-rust unified-crate with the latest commits on ecere/dggal and ecere/eC #5
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
base: unified-crate
Are you sure you want to change the base?
Conversation
I removed this because (a newer version of?) rust complained that the |
MichaelJendryke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to place dggal in a context like this
use once_cell::sync::Lazy;
use std::env;
use std::sync::Mutex;
use dggal_rust::dggal::DGGAL;
use dggal_rust::ecrt::Application;
pub static GLOBAL_APP: Lazy<Mutex<Application>> = Lazy::new(|| {
let args = env::args().collect();
let app = Application::new(&args);
Mutex::new(app)
});
pub static GLOBAL_DGGAL: Lazy<Mutex<DGGAL>> = Lazy::new(|| {
let app = GLOBAL_APP.lock().expect("Failed to lock GLOBAL_APP");
let dggal = DGGAL::new(&*app);
Mutex::new(dggal)
});by adding Send + Sync to Application, DGGAL and DGGRID
| unsafe impl Sync for DGGRS {} | ||
| unsafe impl Send for DGGRS {} | ||
| unsafe impl Sync for DGGAL {} | ||
| unsafe impl Send for DGGAL {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jerstlouis I had a few issues to get tests to work concurrently and I just expanded your commit to also wrap DGGAL with Send + Sync. Do you think it is okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaelJendryke To run the initial example concurrency test that was not necessary.
Could you please clarify what the new test is doing?
What is this new lazy mutex stuff doing?
The DGGRS class is already thread safe and none of the methods modifies the object state, so they can freely be executed in parallel without locking any mutex.
The Application class is intended to be a singleton.
Instantiating multiple Application class will likely break things at this point.
Are you trying to use Application in multiple threads?
Since it's only used for instantiating DGGAL, which is then use to instantiate the DGGRS(s), this would ideally all be done in the main thread during initialization stage (though I don't think it would cause any problem to instantiate a new DGGRS from the same DGGAL from other threads as needed later on).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this Lazy stuff is just to try to avoid the user having to initialize this thing themselves and ensure there is global singleton set up for these objects? I think I actually had something like this in the bindings for an earlier version.
I remember being advised against this when developing the Rust bindings, but I understand doing this if this is to avoid GeoPlegma users having to do this themselves since it's specific to a particular backend.
| unsafe impl Sync for Application {} | ||
| unsafe impl Send for Application {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jerstlouis also here for the Application
|
When running these dummy tests in GeoPlegma, I can only run this with Which may be an indication, that I cannot run I am wondering if not only The setup that works using 1 thread , is this pub struct DggalImpl {
pub grid_name: String,
}
impl DggalImpl {
pub fn new(grid_name: &str) -> Self {
Self {
grid_name: grid_name.to_string(),
}
}
}
fn get_dggrs(grid_name: &str) -> Result<DGGRS, DggalError> {
let args: Vec<String> = env::args().collect();
let my_app = Application::new(&args);
let dggal = DGGAL::new(&my_app);
DGGRS::new(&dggal, grid_name).map_err(|_| DggalError::UnknownGrid {
grid_name: grid_name.to_string(),
})
}
impl DggrsPort for DggalImpl {
fn zones_from_bbox(
&self,
depth: u8,
densify: bool,
bbox: Option<Vec<Vec<f64>>>,
) -> Result<Zones, PortError> {
let dggrs = get_dggrs(&self.grid_name)?;
.
..
...However, I would rather like to have a context that is instantiating the use once_cell::sync::Lazy;
use std::env;
use std::sync::Mutex;
use dggal_rust::dggal::DGGAL;
use dggal_rust::ecrt::Application;
pub static GLOBAL_APP: Lazy<Mutex<Application>> = Lazy::new(|| {
let args = env::args().collect();
let app = Application::new(&args);
Mutex::new(app)
});
pub static GLOBAL_DGGAL: Lazy<Mutex<DGGAL>> = Lazy::new(|| {
let app = GLOBAL_APP.lock().expect("Failed to lock GLOBAL_APP");
let dggal = DGGAL::new(&*app);
Mutex::new(dggal)
});and than in my code I can use this: pub struct DggalImpl {
pub grid_name: String,
}
impl DggalImpl {
pub fn new(grid_name: &str) -> Self {
Self {
grid_name: grid_name.to_string(),
}
}
}
use crate::adapters::dggal::context::GLOBAL_DGGAL;
fn get_dggrs(grid_name: &str) -> Result<DGGRS, DggalError> {
let dggal = GLOBAL_DGGAL.lock().map_err(|_| DggalError::LockFailure)?;
DGGRS::new(&*dggal, grid_name).map_err(|_| DggalError::UnknownGrid {
grid_name: grid_name.to_string(),
})
} |
Do you have a call stack for where this SIGSEGV occurs? Is it in the GeoPlegma adapter, inside the DGGAL Rust bindings code, or inside the DGGAL eC code? I will try to run the tests myself in a couple days or so when I have a bit more time, but in the meantime, from the eC DGGAL perspective, you can invoke any method of the same DGGRS object instance from as many threads as you like without any issue, and without any locking (internal or external). That is because none of the DGGRS methods take a mutable 'self', and none of the methods will modify the internal DGGRS eC object either (DGGAL doesn't construct / cache zones as you request them, it always computes everything on the fly as you invoke the methods). I think you should also be able to safely instantiate new DGGRS objects from separate threads (from the same DGGAL / Application singleton object), but this really should not be done -- an instance for particular DGGRS (e.g., IVEA3H) should just be created once in one thread (e.g., the main thread) and re-used across all other threads. So I somehow doubt that these SIGSEGV has something to do with the DGGAL code or the bindings if Rust happily compiles it without warnings or errors. But a proper call stack might help understanding the cause of the crash. |
Just an update to include the latest changes. However the compiler complains about an unstable library feature
integer_sign_cast@jerstlouis, I see that you removed the
unsafe { transmute(value) }, any idea?