-
Notifications
You must be signed in to change notification settings - Fork 2
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
Treat syscalls as nodes #112
base: main
Are you sure you want to change the base?
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.
Did you check if it doesn't mess up the inlining? It shouldn't since the syscalls are always leaves in the tree, but it's worth verifying
function_name, | ||
))) = call_stack.last() | ||
{ | ||
let Ok(syscall) = map_syscall_name_to_selector(function_name.0.as_str()) else { |
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.
Imo we should expect
here, if the TryInto
fails it means we have broken the conversion implementation (and names of these libfuncs in sierra shouldn't change, so we don't have to worry about that)
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.
What about other libfuncs? at this level they are indistinguishable from syscalls. I guess expecting here would mean that each time we run at a libfunc that is not in artifacts file, we panic.
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.
At this level they are, but you explicitly add only syscalls there during trace building. Good point though, having the type checked during creation is better, we could do:
pub enum InternalFunctionCall {
Inlined(FunctionName),
NonInlined(FunctionName),
Syscall(DeprecatedSyscallSelector),
}
so it is typesafe
.execute_syscalls | ||
.get(&syscall) | ||
.unwrap_or_else(|| { | ||
panic!("Missing syscall {syscall:?} from versioned constants file") |
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.
We should validate if all syscalls' costs are present in the versioned constants file when reading it so we can unwrap safely 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.
What would be the upside of this approach? I get users might fiddle with the file so technically this might be viewed as user error, but generally speaking if it's missing (and they do not pass a custom map), it's our fault right?
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.
If they pass a custom map with missing syscalls: it is a user error and we should report it as early as possible (during reading of the file)
If it is missing at this point in the code it is our fault, that's why unwrapping here is "safe" (if it fails i means we don't handle a certain syscall)
This comment may be in a wrong place, I just thought about it here (since it is the only way to make this unwrap make sense), sorry xD
c690afe
to
1086f92
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.
Did you check if it doesn't mess up the inlining? It shouldn't since the syscalls are always leaves in the tree, but it's worth verifying
Yup, when I was testing I did it for both inlining and non-inlining, and compared to the same artifacts using code from main, nothing looked out of place or suspicious
crates/cairo-profiler/src/trace_reader/function_trace_builder.rs
Outdated
Show resolved
Hide resolved
crates/cairo-profiler/src/trace_reader/function_trace_builder.rs
Outdated
Show resolved
Hide resolved
Ok(CoreConcreteLibfunc::StarkNet(_)) => { | ||
if invocation.libfunc_id.debug_name.is_none() { | ||
// this libfunc is not included in the artifact file | ||
// it is likely to be a libfunc from the test itself |
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.
Some libfuncs seem to not be included in the sierra file, even though they are executed and can be catched here.
An example of such libfuncs can be, for example, a ContractAddressTryFromFelt252
or just Testing(Cheatcode)
that is being called by the test (snforge)
|
||
if !in_syscall { | ||
in_syscall = true; | ||
let mut new_current_call_stack = current_call_stack.clone(); |
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.
Yup, this just means that we need to mark current_call_stack
as mut, and I wasn't sure it's the best way as call stack is typically not very large.
If you prefer it to be mut I can change, do not have strong opinion on this tbh
function_name, | ||
))) = call_stack.last() | ||
{ | ||
let Ok(syscall) = map_syscall_name_to_selector(function_name.0.as_str()) else { |
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.
What about other libfuncs? at this level they are indistinguishable from syscalls. I guess expecting here would mean that each time we run at a libfunc that is not in artifacts file, we panic.
.execute_syscalls | ||
.get(&syscall) | ||
.unwrap_or_else(|| { | ||
panic!("Missing syscall {syscall:?} from versioned constants file") |
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.
What would be the upside of this approach? I get users might fiddle with the file so technically this might be viewed as user error, but generally speaking if it's missing (and they do not pass a custom map), it's our fault right?
@@ -220,3 +257,84 @@ fn build_current_call_stack( | |||
current_call_stack | |||
} | |||
} | |||
|
|||
fn stack_trace_to_samples( |
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.
Pls extract this function and the one below to separate module in crates/cairo-profiler/src/trace_reader/function_trace_builder/
, this file is getting big
Closes #88
Introduced changes
Checklist
CHANGELOG.md
Minimal example: