Stop creating Owner Stacks if many have been created recently #32529
Conversation
github-actions
bot
added
the
React Core Team
|
|
||
| let recentlyCreatedOwnerStacks = 0; | ||
| let recentlyCreatedDebugTasks = 0; | ||
| setInterval(() => { |
There was a problem hiding this comment.
Can we move the counter and timer to isomorphic react?
It would also be good to reset the counter every commit (and reset the timer too, but maybe not start it until the next render).
There was a problem hiding this comment.
Do we not mean react with "isomorphic react"?
There was a problem hiding this comment.
This interval will be duplicated across jsx and createElement since they're in separate bundles. So it's two.
However, that seems fine. If you have a run away number of elements you'll have that with either createElement or jsx.
Better than leaking this info outside the module and having to read it from an object.
The reason to do isomorphic would be if it was reset using commit or something in each renderer. Although if you have a bunch of frequent commits, maybe it's not so much better.
It does seem weird to have a this running all the time even when React isn't rendering though.
Also, this needs to be DEV-only gated and feature detected if setInterval exists.
There was a problem hiding this comment.
It does seem weird to have a this running all the time even when React isn't rendering though.
That would require adding a dedicated method to internals, right? Should we just share a method to reset with renderers and they decide when to reset (timer-based, commit based etc.) or should it just be a a simple start/stop interval?
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
f134542 to
735d7cf
Compare
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
4551dfd to
6fbd265
Compare
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
6fbd265 to
bc9f075
Compare
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
bc9f075 to
c7fdaef
Compare
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
c7fdaef to
dbaf445
Compare
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
dbaf445 to
bf81729
Compare
| // 1 for the timeout | ||
| // And since we haven't committed yet, we continue to reset the Owner Stack | ||
| // limit periodically. | ||
| pendingTimers: 2, |
There was a problem hiding this comment.
This is where Fiber differs from Fizz and Flight. prepareFreshStack isn't called when we render from a ping if we never committed. Fizz and Flight will stop at this point in the test though.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
bf81729 to
c69dfdc
Compare
|
|
||
| import ReactSharedInternals from 'shared/ReactSharedInternals'; | ||
|
|
||
| let resetOwnerStackIntervalId: mixed = null; |
There was a problem hiding this comment.
each renderer has its own interval now. Should we just move this timeout ID into isomorphic React?
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
c69dfdc to
1a87f42
Compare
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
de492f5 to
bb33d9b
Compare
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
bb33d9b to
323be7b
Compare
| } else { | ||
| const localDate = Date; | ||
| const initialTime = localDate.now(); | ||
| getCurrentTime = () => localDate.now() - initialTime; |
There was a problem hiding this comment.
You don't need the initialTime offset here. That's just something scheduler does for consistency with performance.now and you can't rely on it.
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
323be7b to
4f95516
Compare
Add prestart
Adding Owner Stacks in development adds some non-negligible performance overhead. This is shared roughly equally between the work required for `captureOwnerStack` to work and tasks being visible in runtimes that support `console.createTask`. We now stop this work after a some amount of elements were created. Though we do reset that limit occasionally so that one-off updates on not too large trees do get complete Owner Stacks. Chances are that you probably don't need Owner Stacks on large, frequent updates. Stopping after 10.000 elements caps a large update at 500ms where uncapped would've taken 3.000ms (see attached fixture). I added separate, dynamic feature flags to control when we cut off and how frequent we reset so that Meta can experiment with different values.
This reverts commit 97ffdbb. No longer needed now that we stop resetting outside render.
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
4f95516 to
d615a3d
Compare
Has the benefit of not suddenly resetting in the middle of render
eps1lon
force-pushed
the
sebbie/owner-stack-cutoff
branch
from
d615a3d to
54e6c53
Compare
Co-authored-by: Jack Pope <[email protected]> DiffTrain build for [4a9df08](4a9df08)
Co-authored-by: Jack Pope <[email protected]> DiffTrain build for [4a9df08](4a9df08)
#32529 added a dynamic flag for this, but that breaks tests since the flags are not defined everywhere. However, this is a static value and the flag is only for supporting existing tests. So we can override it in the test config, and make it static at built time instead.
#32529 added a dynamic flag for this, but that breaks tests since the flags are not defined everywhere. However, this is a static value and the flag is only for supporting existing tests. So we can override it in the test config, and make it static at built time instead. DiffTrain build for [f99c9fe](f99c9fe)
#32529 added a dynamic flag for this, but that breaks tests since the flags are not defined everywhere. However, this is a static value and the flag is only for supporting existing tests. So we can override it in the test config, and make it static at built time instead. DiffTrain build for [f99c9fe](f99c9fe)
Summary
Adding Owner Stacks in development adds some non-negligible performance overhead.
This is shared roughly equally between the work required for
captureOwnerStackto work and tasks being visible in runtimes that supportconsole.createTask.We now stop this work after a some amount of elements were created. Though we do reset that limit occasionally during render so that one-off updates on not too large trees do get complete Owner Stacks. A new render will always start fresh.
Chances are that you probably don't need Owner Stacks on large, frequent updates.
Stopping after 10.000 elements caps a large update at 500ms where uncapped would've taken 3.000ms (see attached fixture).
How did you test this change?