-
Notifications
You must be signed in to change notification settings - Fork 923
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
Implements Javascript profiling support. #371
base: master
Are you sure you want to change the base?
Conversation
…qjs outputs chrome devtools-compatible profiling data
By the way, the current implementation in the If |
This is excellent, thanks Rubén. You published it just in time for something I needed. I ran into some issues probably due to embedding quickjs through quickjspp instead of running in qjs. IssuesQuickjspp adds C++-side functions to JS as getters, I think. My program does reentry C++ loop -> JS frame -> C++ bound funcs, in case that's relevant. I worked around that sanitizing for the time being because I didn't have time to dig into the qjs codebase and figure out what went wrong. Definitely not great for overhead but still helped me a lot.
ToolsFirst the good news, ui.perfetto.dev works perfectly well, even with a lot of data and some OOM crashes. Chrome Tools' profiler on the other hand is really bad at loading the profile. I’m getting: Patching quickjsppAs quickjspp integrates quickjs manually without subtrees/submodules I'll leave this script to patch and build quickjspp for reference in case anyone wants to try this great profiler out and is in the same situation as I am.
|
I also had problems with loading large profile traces in Chrome Profiler. I ended up creating a tree of calls in memory and pruning the fast branches (those that take less than 0.1 ms to execute) before writing the file. This made the file much smaller and more manageable. I didn't do any of that for the qjs binary in this PR, though. The garbled names could be caused by how quickjspp maps the C++ API to quickjs. I can't reproduce that myself. When I expose C++ functions and classes to quickjs, I control the names I give them, and that's what I get back in the profiles. I also do multiple jumps between C++ and Javascript and yet the profiles are clean. |
I had some time today and I figured out the issue,
|
Thanks @2bam I'll take a look after vacation. |
" --memory-limit n limit the memory usage to 'n' bytes\n" | ||
" --stack-size n limit the stack size to 'n' bytes\n" | ||
" --unhandled-rejection dump unhandled promise rejections\n" | ||
"-q --quit just instantiate the interpreter and quit\n"); | ||
exit(1); | ||
} | ||
|
||
#ifdef CONFIG_PROFILE_CALLS | ||
void profile_function_start(JSContext *ctx, JSAtom func, JSAtom filename, void *opaque) { | ||
const char* func_str = JS_AtomToCString(ctx, func); |
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.
Here the func
parameter can receive a JS_ATOM_NULL (passed on from a get_full_func_name
returned value), and JS_AtomToCString
will not do a null check causing unexpected behavior.
Checking before (like something similar to the line below) will solve the issue.
Same goes for profile_function_end
.
This pull request addresses #183 and builds upon the ideas introduced in #184, taking a slightly different approach to enhance flexibility and functionality:
<:isGone:>
. This implementation resolves that by reporting profiling data at function entry and exit, ensuring the function name remains valid. Additionally, this approach includes context for functions (e.g., the constructor of thethis
object, akin to V8 stack traces for disambiguation) and flags anonymous functions explicitly../qjs -p profile.json examples/pi_bigint.js 20
and then opening profile.json in the Chrome Dev Tools:Please note that I'm not super familiar with QuickJS yet. I downloaded the code this week and have been playing with it only for a couple of days. Let me know if there are better ways to do some of the things I'm doing here, or if there is a coding style guide this change should adhere to.
Thanks!