Track mounted instances with ReactDebugInstanceMap #6486
Conversation
This is a follow-up to #6389, also extracted from #6046. In #6046, we added a new API for associating internal instances with debug-time IDs. Third-party tools such as React DevTools and ReactPerf would use those IDs to query information about the instances, rather than use the instances directly. Here, we add the integration of `ReactDebugInstanceMap` into `ReactDOM` and `ReactDOMServer` rendering. The instances are registered during instantiation because we need to know their IDs right away: some events, such as “we are calling the constructor”, may fire before the instance is mounted. Currently we unregister instances when they get unmounted but in the future it will be possible to separate unmounting from unregistration, in case we ever want to support reparenting. One notable change here is that we now call `unmountComponent()` for server rendering in `__DEV__`. Traditionally this has not been necessary, as server rendering code path in mounting doesn’t allocate any resources it needs to release. However, now that we track every component instantiation, we also want to remove this information, so this requires a real unmounting pass on the server. Nevertheless, since this change is only relevant for `__DEV__`, it does not affect the production performance. Some existing code in `unmountComponent()` used to rely on the fact that it’s only called on the client. To mitigate this, I added a flag called `hasReactMountReady` to the transactions. It indicates whether `getReactMountReady()` is a real queue or a no-op. This way, component can tell, for example, whether it needs to run `componentWillUnmount()`, or whether it needs to ensure the node is allowed to be unmounted, such as in case of `<html>`. One concern I have is that conditionally running `unmountComponent()` for server rendering can make it easy to introduce accidental memory leaks, as it will always run in the test environment but not in production. An alternative option would be to add a separate method called `releaseComponent()` that serves as `unmountComponent()` during server rendering only. This way relying on `unmountComponent()` deallocating a resource will be easy to spot because `unmountComponent()` still won’t run in tests for server rendering. Another concern is that we’re finally using `ReactDebugInstanceMap` in the code which means we now have a dependency on WeakMap being available in `__DEV__` builds. If this is bad, we should do something about it.
|
Do you think adding a special |
| emitEvent('onSetState'); | ||
| }, | ||
| onInstantiateComponent(internalInstance) { | ||
| ReactDebugInstanceMap.registerInstance(internalInstance); |
There was a problem hiding this comment.
Remind me again why this needs to be here and not in a DevTool that listens to the event below?
If it was attached conditionally by a devtool, then we wouldn't take on a dependency on WeakMap. Only in environments that needs that tool.
We definitely can't take on a dependency on WeakMap for all environments. Would make it impossible debug many IE versions.
There was a problem hiding this comment.
I want to make sure this happens before any devtool receives the event. (Since devtools are actually going to get IDs—I will implement this separately later but #6046 shows how it could work).
I also want to have the opposite guarantee for unmounting: that ID gets unregistered after all the other devtools have run. This is why putting such a devtool as first in list wouldn’t work.
|
I like your idea of a separate clean up event better than firing unmountComponent. We're very strict on never ever changing behavior between dev and prod. It is very dangerous. I'm not sure about even that though. It is likely that the server renderer will become streaming and it won't even have an internal tree at all. We won't be able to unmount because there is no tree in memory. We'd just execute one component after another and stream the result out. The only thing that could have the stateful tree is the DebugInstanceMap. I'm not sure why you need this at all though. It seems to me that debugging server rendering is only going to be useful for like a single page request and you want to isolate the scope. You might want multiple roots but you don't want to have different page requests in your debug tool. Couldn't we just blow out the entire tree from the debug instance map once the response has finished? Come to think of it. For a completely stateless component tree we also wouldn't have a tree to unmount so this would have to work the same for such trees. I.e. when a root gets released, you use the debug tree to clean up itself. With WeakRef finalizers (in progress in spec) this whole thing of managing remote references becomes a lot easier. Btw, React Native has the same problem because native objects are referenced by IDs (tags). I actually wanted to stop using that and just use pointers + finalizers instead. No need for giant and slow map lookups. That would also eliminate the need for React to keep a stateful tree just so that it can call unmount on everything. |
I tried that at first but then I realized this doesn’t work for cases when people call With the current behavior, ReactPerf should be able to measure time correctly even if you mix client and server rendering. This test verifies it. |
|
|
|
I’m confused, isn’t removing the entire subtree pretty much what I’m doing in this PR? |
|
Sorry. By "removing the entire subtree", I mean broadcasting a debug tool event that says something like "unregisterSubtree(root)". I.e. the difference is that you don't traverse the internal tree inside React. |
|
How would it know which children to remove from the map? It doesn’t currently have the information about the tree, just about separate instances. I’m adding The plan was to make I don’t quite see why the current implementation would be problematic for a streaming renderer, for example. It would just call Internal instances can also be phased out gradually because the API we actually expose to devtools would only deal with IDs, so we can hide arbitrary data behind those IDs. In fact we already have an example of a “virtual” ID that doesn’t correspond to its own instance: a fake single child representing an inlined text node in a native component. As long as we can answer questions like Does this make sense? |
|
Another alternative to storing a replica of the whole tree is storing one instancesByIDs map per root and then just kill the whole root. You would probably want to encode the root in the ID. E.g. by using two numbers or just use some upper or lower bit range that is used to identify the root. That way you can use that number to look up which map to use when instancesByIDs is called. Or you can merge it into a single thing together with We can do this now or later. Regardless we'll have to do the work of storing an external shadow tree pretty soon. Otherwise we're going to be immediately blocking reconciler work on updating the debug tools. That's why I think we might as well solve it now rather than adding a dangerous API change (unmount firing on DEV servers). Does that make sense? |
Ah, that makes sense. I’ll give it a try! |
|
@sebmarkbage Does f99fc80 look better w/r/t not using |
|
@gaearon updated the pull request. |
|
Yea, that's great. Why a string instead of a number though? Shouldn't we preserve the more efficient data structure as long as possible and only convert it to strings if the transport protocol requires it? |
|
Fair enough! We can’t it use as an “official” ID anyway because we want to embed more info (such as root ID). So I guess we can stringify at that point instead. |
|
Why can't the root ID be embedded in the official ID? Isn't that just an implementation detail? |
|
Do you mean that instead of generating it here as a naïve counter, we would thread |
|
My idea was much more naive but that sounds good too. We do pass |
|
It is unfortunate that we have to accommodate these IDs in the core though. Maybe I'm just tricking myself into thinking that it will help with decoupling. :/ |
|
In an environment that has expandos on native nodes (DOM) or weakref finalizers (like React Native). None of this is a problem because you just let the GC deal with it. I don't think it is very valuable to separate IDs from internal instances for privacy reasons. We could just use the internal instance as the ID. As soon as you build an RPC system on top of this you'll have to deal with the problem. Such as the Chrome devtools. That's the only reason I think this problem is worth solving at this level at all. So that others don't have to deal with these issues. |
|
Closing per conversation with @sebmarkbage. I’ll change the approach to always pushing data from the React core into devtools so that they can never query it back. This way we the API can stay the same even if instances don’t really exist, or if they don’t have clearly defined children to “read” at a specific moment in time. |
3 participants
This is a follow-up to #6389, also extracted from #6046.
In #6046, we added a new API for associating internal instances with debug-time IDs. Third-party tools such as React DevTools and ReactPerf would use those IDs to query information about the instances, rather than use the instances directly.
Here, we add the integration of
ReactDebugInstanceMapintoReactDOMandReactDOMServerrendering. The instances are registered during instantiation because we need to know their IDs right away: some events, such as “we are calling the constructor”, may fire before the instance is mounted. Currently we unregister instances when they get unmounted but in the future it will be possible to separate unmounting from unregistration, in case we ever want to support reparenting.One notable change here is that we now call
unmountComponent()for server rendering in__DEV__. Traditionally this has not been necessary, as server rendering code path in mounting doesn’t allocate any resources it needs to release. However, now that we track every component instantiation, we also want to remove this information, so this requires a real unmounting pass on the server. Nevertheless, since this change is only relevant for__DEV__, it does not affect the production performance.Some existing code in
unmountComponent()used to rely on the fact that it’s only called on the client. To mitigate this, I added a flag calledhasReactMountReadyto the transactions. It indicates whethergetReactMountReady()is a real queue or a no-op. This way, component can tell, for example, whether it needs to runcomponentWillUnmount(), or whether it needs to ensure the node is allowed to be unmounted, such as in case of<html>.One concern I have is that conditionally running
unmountComponent()for server rendering can make it easy to introduce accidental memory leaks, as it will always run in the test environment but not in production. An alternative option would be to add a separate method calledreleaseComponent()that serves asunmountComponent()during server rendering only. This way relying onunmountComponent()deallocating a resource will be easy to spot becauseunmountComponent()still won’t run in tests for server rendering.Another concern is that we’re finally using
ReactDebugInstanceMapin the code which means we now have a dependency onWeakMapbeing available in__DEV__builds. If this is bad, we should do something about it.