Only capture stacks for up to 10 frames for Owner Stacks #34864
Conversation
github-actions
bot
added
the
React Core Team
| let debugStackDEV = false; | ||
| if (__DEV__) { | ||
| if (trackActualOwner) { | ||
| if (supportsSettingStackTraceLimit) { | ||
| try { | ||
| const previousStackTraceLimit = Error.stackTraceLimit; | ||
| Error.stackTraceLimit = ownerStackTraceLimit; | ||
| debugStackDEV = Error('react-stack-top-frame'); | ||
| Error.stackTraceLimit = previousStackTraceLimit; | ||
| } catch {} | ||
| } | ||
| if (!debugStackDEV) { | ||
| debugStackDEV = Error('react-stack-top-frame'); | ||
| } | ||
| } else { | ||
| debugStackDEV = unknownOwnerDebugStack; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same approach everywhere but I didn't create a helper to save a stackframe. gcc doesn't have a @inline directive afaict. Only @noinline
There was a problem hiding this comment.
Yea, and I'm actually concerned about the getOwner call which is too large to get inlined. We're missing a lot of inlining here.
|
Hermes doesn't implement this field, so it should be safe. We didn't observe any performance issues with Error object construction. |
There was a problem hiding this comment.
Too much. I'm concerned about just these lines being too much:
const previousStackTraceLimit = Error.stackTraceLimit;
Error.stackTraceLimit = ownerStackTraceLimit;
debugStackDEV = Error('react-stack-top-frame');
Error.stackTraceLimit = previousStackTraceLimit
We shouldn't add a single line of code more to the actual runtime.
| let debugStackDEV = false; | ||
| if (__DEV__) { | ||
| if (trackActualOwner) { | ||
| if (supportsSettingStackTraceLimit) { | ||
| try { | ||
| const previousStackTraceLimit = Error.stackTraceLimit; | ||
| Error.stackTraceLimit = ownerStackTraceLimit; | ||
| debugStackDEV = Error('react-stack-top-frame'); | ||
| Error.stackTraceLimit = previousStackTraceLimit; | ||
| } catch {} | ||
| } | ||
| if (!debugStackDEV) { | ||
| debugStackDEV = Error('react-stack-top-frame'); | ||
| } | ||
| } else { | ||
| debugStackDEV = unknownOwnerDebugStack; | ||
| } | ||
| } |
There was a problem hiding this comment.
Yea, and I'm actually concerned about the getOwner call which is too large to get inlined. We're missing a lot of inlining here.
| Error.stackTraceLimit = ownerStackTraceLimit; | ||
| debugStackDEV = Error('react-stack-top-frame'); | ||
| Error.stackTraceLimit = previousStackTraceLimit; | ||
| } catch {} |
There was a problem hiding this comment.
try/catch is notorious for deopting. It's better in modern browsers but can still trigger extra stuff on the stack and avoid inlining capabilities by making larger code.
We need to cut everything. We should just let it throw if this is not supported.
| let debugStackDEV = false; | ||
| if (__DEV__) { | ||
| if (trackActualOwner) { | ||
| if (supportsSettingStackTraceLimit) { |
There was a problem hiding this comment.
I'm concerned about this unnecessary condition adding to code size (in terms of memory / JIT optimization and not download size) and the extra lookup every call.
We should make two entire copies of the function and switch which function we use instead so that the function that we use doesn't have to make any additional checks at runtime.
Alternatively we should just assume that you can set it and not support environments that don't play ball.
tv off
eps1lon
deleted the
sebbie/10-15-only_capture_debug_stacks_for_up_to_10_frames
branch
4 participants
Turns out
Error.stackTraceLimitdoes affectError()performance (in v8). But only if you're not inspecting with a debugger (e.g. ChromeDevTools is closed). Where this matters more is for SSR where you commonly aren't actively inspecting. Safari (JSC) and Firefox (SpiderMonkey) seem to have no issue with a high stack trace limit. JSC even defaults to 100 while Firefox doesn't even haveError.stackTraceLimit.A higher limit is nice though for showing sync stacks. Especially with ignore-listing, a high limit can still produce nice stacks when most of the frames end up in 3rd party code. For Owner Stacks, we usually don't need very deep stacks. You mostly just have a
.maporuseMemothat creates the JSX.So now we're setting the limit to 10 (v8 default) which allows frameworks to use a higher limit for nice sync stacks without severely degrading performance due to React Owner Stacks. 5 didn't show a meaningful difference in apps exhausting the Owner Stack limit of 10k in SSR.
The ownerstack fixture won't show the improvement since that already uses the default stack trace limit.
One thing to keep an eye out is Hermes where I haven't tested it. Only v8, JSC and SpiderMonkey.