-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(node): experimental trace support in @swc/core
#3731
Conversation
@@ -77,6 +79,8 @@ pub fn print( | |||
|
|||
#[napi] | |||
pub fn print_sync(program: String, options: Buffer) -> napi::Result<TransformOutput> { | |||
crate::util::init_trace_once(false, None)?; |
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.
This repeated call is to preserve existing behavior to support ENV_LOG
without breaking changes.
I hope to declare breaking changes to this eventually, by letting users explicitly opt in by calling trace registration.
@@ -69,6 +70,7 @@ struct Arrow { | |||
impl VisitMut for Arrow { | |||
noop_visit_mut_type!(); | |||
|
|||
#[tracing::instrument(skip_all)] |
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.
Instead of manually creating span or use span! macro with hardcoded name, we'll use #instrument
attr in general. We can always use span! or manual span for the cases we need additional customization but as a first step decorating all of our overridden visitor with default span would makes sense.
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.
I think this can make swc marginally slower.
v1.2.145: https://github.com/swc-project/swc/runs/5316958723?check_suite_focus=true
Current branch: https://github.com/swc-project/swc/runs/5325695105?check_suite_focus=true
Yes, there's overhead to enable tracings on runtime binary. That is also noted in the original proposal as well. It is not uncommon though, large runtimes like chromium enables trace by default as it gives rich data for the performances on each user's machine. Overhead is a cost for those tradeoffs The other alternative is not user-friendly in my opinion - either users have to opt in a specific binary, or run a native profiler which involves non-trivial setup for normal users. That's where I came to think we may be able to bite those cost. |
Also per tokio-rs/tracing#1070, it looks like explicitly opt in specific trace level reduces those overhead. Updated PR to use |
This reverts commit 7c92703.
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.
LGTM.
Thank you!
swc-bump:
- swc_ecma_transforms_compat
Description:
This PR exposes new experimental interface
__experimental_registerGlobalTraceConfig
in@swc/core
, allows generating event trace format based trace output for performance diagnose. Rust / node based CLI doesn't expose this flags yet.The idea is pretty much same as #3066, PR adds some polishing & internal structures. Exposed interface itself may need some iteration, however it is exposed as
__experimental
for now. PR implements trace spans fores2015::arrow
, generated trace confirms it is correct span with correct module (viacategory
).Notable changes in this PR is 40a96ae. I did add instrumentation spans for all of default
visit_*
,fold_*
viaswc_ecma_visit_macro
and then back it out. Codewise it worked, but in practical it creates too verbose traces makes trace output unusable. For example, with macro support running transform over 150 file created trace larger than 700MB. It is nearly not possible to load on trace viewer as well.In result, proposed approach in here is 9b2adee
#[instrument]
attrOnly thing to note is I choose to use
skip_all
to not to include fn param into trace. Those are typically AST nodes, large enough to increase trace size.Related issue (if exists):