Skip to content

Run R tasks on the R thread #109

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

Merged
merged 27 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e5eb1b5
Add `r_task()` and run environment tasks on the main thread
lionel- Oct 6, 2023
2b85a42
Log time waiting and prevent interrupts
lionel- Oct 9, 2023
cd1ba2d
Prepare for an uninitialized R thread
lionel- Oct 9, 2023
8b33fe9
Implement `r_safely!` in harp and use it in session API
lionel- Oct 9, 2023
947eba8
Run `read_console()` within `safely!`
lionel- Oct 9, 2023
3fe8b4f
Use `r_task()` in more places
lionel- Oct 9, 2023
7ddf8de
Run tasks at interrupt time
lionel- Oct 9, 2023
196e52a
Add escape hatch in `r_task()` for unit tests
lionel- Oct 9, 2023
bfed682
Add timeout in `acquire_r_main()`
lionel- Oct 10, 2023
e1abbd4
Remove `test_threads()`
lionel- Oct 10, 2023
a1438d2
Disable `library()` diagnostics to avoid `r_lock!`
lionel- Oct 10, 2023
aa12f24
Relax `Send` requirement on `r_task()` closure
lionel- Oct 10, 2023
fd2d19c
Remove remaining `r_lock!` contexts
lionel- Oct 10, 2023
c9ccd4d
Use `SEXP` objects in R tasks again
lionel- Oct 10, 2023
83435cb
Reformat with +nightly rustfmt
lionel- Oct 10, 2023
b019126
Apply suggestions from code review
lionel- Oct 11, 2023
2be6acb
Restore `Send` bound on task closures
lionel- Oct 11, 2023
d4be1d3
Always check for interrupts in `read_console()` even when suspended
lionel- Oct 11, 2023
bbb8b79
Remove redundant interrupt disabling
lionel- Oct 11, 2023
865365e
Run maximum 3 tasks at a time at yield-time
lionel- Oct 11, 2023
287537a
Refactor `read_console()` loop
lionel- Oct 11, 2023
f66e0e9
Only run tasks within `yield_to_tasks()`
lionel- Oct 11, 2023
d56118c
Add initial logging when requesting a task
lionel- Oct 11, 2023
63b4c5f
Expand notes on `r_task()` safety
lionel- Oct 11, 2023
ad4b1e2
Remove dangling `r_safely`
lionel- Oct 11, 2023
1ab04b7
Make `r_safely()` a function rather than a macro
lionel- Oct 11, 2023
7d0ebeb
Reintroduce a simple lock for unit tests
lionel- Oct 11, 2023
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
29 changes: 20 additions & 9 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
[workspace]

# Was necessary after switching to dev tree-sitter to fix this warning:
# > some crates are on edition 2021 which defaults to `resolver = "2"`, but
# > virtual workspaces default to `resolver = "1"`
resolver = "2"

members = [
"crates/amalthea",
"crates/ark",
Expand Down
6 changes: 3 additions & 3 deletions crates/ark/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ nix = { version = "0.26.2", features = ["signal"] }
notify = "6.0.0"
once_cell = "1.17.1"
parking_lot = "0.12.1"
regex = "1.7.1"
regex = "1.10.0"
reqwest = { version = "0.11.20", features = ["json"] }
ropey = "1.6.0"
rust-embed = "8.0.0"
Expand All @@ -43,8 +43,8 @@ serde_json = "1.0.94"
stdext = { path = "../stdext" }
tokio = { version = "1.26.0", features = ["full"] }
tower-lsp = "0.19.0"
tree-sitter = "0.20.9"
tree-sitter-r = { git = "https://github.com/r-lib/tree-sitter-r", branch = "next" }
tree-sitter = { git = "https://github.com/tree-sitter/tree-sitter" }
tree-sitter-r = { git = "https://github.com/r-lib/tree-sitter-r", branch = "bugfix/update" }
uuid = "1.3.0"
url = "2.4.1"
walkdir = "2"
Expand Down
10 changes: 5 additions & 5 deletions crates/ark/src/data_viewer/r_data_viewer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use crossbeam::channel::Sender;
use harp::exec::RFunction;
use harp::exec::RFunctionExt;
use harp::object::RObject;
use harp::r_lock;
use harp::utils::r_assert_length;
use harp::utils::r_is_data_frame;
use harp::utils::r_is_matrix;
Expand Down Expand Up @@ -44,6 +43,7 @@ use crate::data_viewer::message::DataViewerMessageRequest;
use crate::data_viewer::message::DataViewerMessageResponse;
use crate::data_viewer::message::DataViewerRowRequest;
use crate::data_viewer::message::DataViewerRowResponse;
use crate::r_task;

pub struct RDataViewer {
title: String,
Expand Down Expand Up @@ -195,7 +195,7 @@ impl DataSet {
}

pub fn from_object(id: String, title: String, object: RObject) -> Result<Self, anyhow::Error> {
r_lock! {
r_task(|| unsafe {
let row_count = {
if r_is_data_frame(*object) {
let row_names = Rf_getAttrib(*object, R_RowNamesSymbol);
Expand All @@ -214,10 +214,10 @@ impl DataSet {
Ok(Self {
id: id.clone(),
title: title.clone(),
columns: columns,
row_count: row_count
columns,
row_count,
})
}
})
}

fn slice_data(&self, start: usize, size: usize) -> Result<Vec<DataColumn>, anyhow::Error> {
Expand Down
59 changes: 22 additions & 37 deletions crates/ark/src/environment/r_environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use harp::environment::Environment;
use harp::exec::RFunction;
use harp::exec::RFunctionExt;
use harp::object::RObject;
use harp::r_lock;
use harp::utils::r_assert_type;
use harp::vector::CharacterVector;
use harp::vector::Vector;
Expand All @@ -40,6 +39,7 @@ use crate::environment::message::EnvironmentMessageUpdate;
use crate::environment::message::EnvironmentMessageView;
use crate::environment::variable::EnvironmentVariable;
use crate::lsp::events::EVENTS;
use crate::r_task;

/**
* The R Environment handler provides the server side of Positron's Environment
Expand Down Expand Up @@ -218,14 +218,13 @@ impl REnvironment {
fn refresh(&mut self, request_id: Option<String>) {
let mut variables: Vec<EnvironmentVariable> = vec![];

r_lock! {
r_task(|| {
self.update_bindings(self.bindings());

for binding in &self.current_bindings {
variables.push(EnvironmentVariable::new(binding));
}

}
});

// TODO: Avoid serializing the full list of variables if it exceeds a
// certain threshold
Expand All @@ -244,8 +243,7 @@ impl REnvironment {
*/
fn clear(&mut self, include_hidden_objects: bool, request_id: Option<String>) {
// try to rm(<env>, list = ls(envir = <env>, all.names = TRUE)))
let result: Result<(), harp::error::Error> = r_lock! {

let result: Result<(), harp::error::Error> = r_task(|| unsafe {
let mut list = RFunction::new("base", "ls")
.param("envir", *self.env)
.param("all.names", Rf_ScalarLogical(include_hidden_objects as i32))
Expand All @@ -264,7 +262,7 @@ impl REnvironment {
.call()?;

Ok(())
};
});

if let Err(_err) = result {
error!("Failed to clear the environment");
Expand All @@ -281,8 +279,8 @@ impl REnvironment {
* Clear the environment. Uses rm(envir = <env>, list = ls(<env>, all.names = TRUE))
*/
fn delete(&mut self, variables: Vec<String>, request_id: Option<String>) {
r_lock! {
let variables : Vec<&str> = variables.iter().map(|s| s as &str).collect();
r_task(|| unsafe {
let variables: Vec<&str> = variables.iter().map(|s| s as &str).collect();

let result = RFunction::new("base", "rm")
.param("list", CharacterVector::create(variables).cast())
Expand All @@ -292,16 +290,15 @@ impl REnvironment {
if let Err(_) = result {
error!("Failed to delete variables from the environment");
}
}
});

// and then update
self.update(request_id);
}

fn clipboard_format(&mut self, path: &Vec<String>, format: String, request_id: Option<String>) {
let clipped = r_lock! {
EnvironmentVariable::clip(RObject::view(*self.env), &path, &format)
};
let clipped =
r_task(|| EnvironmentVariable::clip(RObject::view(*self.env), &path, &format));

let msg = match clipped {
Ok(content) => {
Expand All @@ -319,9 +316,7 @@ impl REnvironment {
}

fn inspect(&mut self, path: &Vec<String>, request_id: Option<String>) {
let inspect = r_lock! {
EnvironmentVariable::inspect(RObject::view(*self.env), &path)
};
let inspect = r_task(|| EnvironmentVariable::inspect(RObject::view(*self.env), &path));
let msg = match inspect {
Ok(children) => {
let length = children.len();
Expand All @@ -340,9 +335,8 @@ impl REnvironment {
}

fn view(&mut self, path: &Vec<String>, request_id: Option<String>) {
let data = r_lock! {
EnvironmentVariable::resolve_data_object(RObject::view(*self.env), &path)
};
let data =
r_task(|| EnvironmentVariable::resolve_data_object(RObject::view(*self.env), &path));

let msg = match data {
Ok(data) => {
Expand Down Expand Up @@ -386,7 +380,7 @@ impl REnvironment {
let old_bindings = &self.current_bindings;
let mut new_bindings = vec![];

r_lock! {
r_task(|| {
new_bindings = self.bindings();

let mut old_iter = old_bindings.iter();
Expand All @@ -396,25 +390,20 @@ impl REnvironment {
let mut new_next = new_iter.next();

loop {

match (old_next, new_next) {
// nothing more to do
(None, None) => {
break
},
(None, None) => break,

// No more old, collect last new into added
(None, Some(mut new)) => {
loop {
assigned.push(
EnvironmentVariable::new(&new)
);
assigned.push(EnvironmentVariable::new(&new));

match new_iter.next() {
Some(x) => {
new = x;
},
None => break
None => break,
};
}
break;
Expand All @@ -429,7 +418,7 @@ impl REnvironment {
Some(x) => {
old = x;
},
None => break
None => break,
};
}

Expand All @@ -439,25 +428,21 @@ impl REnvironment {
(Some(old), Some(new)) => {
if old.name == new.name {
if old.value != new.value {
assigned.push(
EnvironmentVariable::new(&new)
);
assigned.push(EnvironmentVariable::new(&new));
}
old_next = old_iter.next();
new_next = new_iter.next();
} else if old.name < new.name {
removed.push(old.name.to_string());
old_next = old_iter.next();
} else {
assigned.push(
EnvironmentVariable::new(&new)
);
assigned.push(EnvironmentVariable::new(&new));
new_next = new_iter.next();
}
}
},
}
}
}
});

if assigned.len() > 0 || removed.len() > 0 || request_id.is_some() {
// only update the bindings (and the version)
Expand Down
Loading