-
Notifications
You must be signed in to change notification settings - Fork 463
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
compute queries in on other threads. #6026
Conversation
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.
includes:
https://reviewable.io/reviews/starkware-libs/cairo/6025#-
Reviewable status: 0 of 6 files reviewed, all discussions resolved
1b4980a
to
1257f49
Compare
Will this be used by |
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.
Reviewed 5 of 8 files at r1, all commit messages.
Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @maciektr)
-- commits
line 3 at r1:
rebase
Code quote:
New commits in r1 on 7/16/2024 at 2:49 PM:
- a2ff8ae: Delay error reporting.
crates/cairo-lang-compiler/src/lib.rs
line 190 at r1 (raw file):
} }); }
maybe move out of function for smaller indentation.
Code quote:
fn handle_func_inner(
processed_function_ids: &Mutex<UnorderedHashSet<ConcreteFunctionWithBodyId>>,
snapshot: salsa::Snapshot<RootDatabase>,
func_id: ConcreteFunctionWithBodyId,
) {
if processed_function_ids.lock().unwrap().insert(func_id) {
rayon::scope(move |s| {
let db = &*snapshot;
let Ok(function) = db.function_with_body_sierra(func_id) else {
return;
};
for statement in &function.body {
let Some(related_function_id) =
try_get_function_with_body_id(db, statement)
else {
continue;
};
let snapshot = salsa::ParallelDatabase::snapshot(&*snapshot);
s.spawn(move |_| {
handle_func_inner(
processed_function_ids,
snapshot,
related_function_id,
)
})
}
});
}
crates/cairo-lang-compiler/src/lib.rs
line 209 at r1 (raw file):
let requested_function_ids_copy = requested_function_ids.clone(); rayon::spawn(move || compute_queries(snapshot, requested_function_ids_copy));
include the spawn
in the function - and call the function warmup_db
so it would be easily callable.
Code quote:
rayon::spawn(move || compute_queries(snapshot, requested_function_ids_copy));
Previously, orizi wrote…
It will add indentation, and it won't be obvious that the warmup happens on a separate thread. |
Previously, maciektr (maciektr) wrote…
Ideally, yes, but it is not apparent how to make it work since |
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.
Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @maciektr and @orizi)
Previously, orizi wrote…
rebase
I didn't merge Delay error reporting
, as I think it is a regression unless we merge this PR.
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.
Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @maciektr)
crates/cairo-lang-compiler/src/lib.rs
line 209 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
It will add indentation, and it won't be obvious that the warmup happens on a separate thread.
i already supplied a basic solution for the indentation issue - for the other part - HEAVILY DOC.
and maybe spawn_db_warmup
or something of that sort.
Previously, orizi wrote…
I need |
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.
Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @maciektr)
crates/cairo-lang-compiler/src/lib.rs
line 190 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
I need
s
from the lambda fors.spawn
.
you can just add it to the function arguments - no?
Previously, orizi wrote…
When I try to do that I get: Code snippet: error[E0521]: borrowed data escapes outside of closure
--> crates/cairo-lang-compiler/src/lib.rs:182:33
|
169 | rayon::scope(move |s| {
| -
| |
| `s` declared here, outside of the closure body
| `s` is a reference that is only valid in the closure body
...
182 | / s.spawn(move |_| {
183 | | handle_func_inner(
184 | | s,
185 | | processed_function_ids,
... |
188 | | )
189 | | })
| |__________________________________^ `s` escapes the closure body here |
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.
Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @maciektr)
crates/cairo-lang-compiler/src/lib.rs
line 190 at r1 (raw file):
} }); }
you may consider extracting this into a regular function, and just calling it from the inner function.
Code quote:
if processed_function_ids.lock().unwrap().insert(func_id) {
rayon::scope(move |s| {
let db = &*snapshot;
let Ok(function) = db.function_with_body_sierra(func_id) else {
return;
};
for statement in &function.body {
let Some(related_function_id) =
try_get_function_with_body_id(db, statement)
else {
continue;
};
let snapshot = salsa::ParallelDatabase::snapshot(&*snapshot);
s.spawn(move |_| {
handle_func_inner(
processed_function_ids,
snapshot,
related_function_id,
)
})
}
});
}
crates/cairo-lang-compiler/src/lib.rs
line 209 at r1 (raw file):
Previously, orizi wrote…
i already supplied a basic solution for the indentation issue - for the other part - HEAVILY DOC.
and maybespawn_db_warmup
or something of that sort.
in any case - just add a wrapper function - won't cause indentation.
Previously, orizi wrote…
I don't follow, where would the rayon scope come from? |
1257f49
to
e592d63
Compare
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.
Reviewed 2 of 4 files at r2, 1 of 4 files at r3.
Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @maciektr)
crates/cairo-lang-compiler/src/lib.rs
line 190 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
I don't follow, where would the rayon scope come from?
would be a param to the external function.
but now i realise this won't actually work in this case.
crates/cairo-lang-compiler/src/lib.rs
line 159 at r3 (raw file):
let processed_function_ids = &Mutex::new(UnorderedHashSet::<ConcreteFunctionWithBodyId>::default()); rayon::scope(move |s| {
how does this compare?
Suggestion:
rayon::scope_fifo(move |s| {
Previously, orizi wrote…
I didn't see a differnece |
e592d63
to
13e6577
Compare
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.
Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @maciektr and @orizi)
crates/cairo-lang-compiler/src/lib.rs
line 190 at r1 (raw file):
Previously, orizi wrote…
would be a param to the external function.
but now i realise this won't actually work in this case.
Done.
crates/cairo-lang-compiler/src/lib.rs
line 209 at r1 (raw file):
Previously, orizi wrote…
in any case - just add a wrapper function - won't cause indentation.
Done.
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.
Reviewed 1 of 4 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @maciektr)
crates/cairo-lang-compiler/src/lib.rs
line 201 at r4 (raw file):
} // Spawns a task to warm up the db.
Suggestion:
/// Spawns a task to warm up the db.
13e6577
to
c1c078c
Compare
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.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @maciektr and @orizi)
crates/cairo-lang-compiler/src/lib.rs
line 201 at r4 (raw file):
} // Spawns a task to warm up the db.
Done.
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.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciektr)
This change is![Reviewable](https://github.com/camo/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)