Tie MultiChild queue to ReactReconcileTransaction #1157
Conversation
|
Opinions here @jordwalke @yungsters @sebmarkbage? |
| errorThrown ? clearQueue() : processQueue(); | ||
| } | ||
| } | ||
| this._updateChildren(nextNestedChildren, transaction); |
There was a problem hiding this comment.
No try-finally here anymore so could bring the function inline again?
There was a problem hiding this comment.
react-art overrides it: https://github.com/facebook/react-art/blob/master/src/ReactART.js#L147-L158. I should update the comment though.
|
I don't yet fully understand the consequences of this change on nested operations and loops such as componentDidUpdate retriggering updates at higher levels. We do want to move to a fully async setState by making all operations batched. For systems which only partially uses React, batching of innerHTML across roots is really important but you may be right that it's already flawed. |
I think it's a cool feature, but there is significant complexity in maintaining it. People who use many react roots won't benefit from a nice perf bump, but I'm not sure how likely it is to be the case that an event will cause several roots to simultaneously need child updates (within the same event loop). |
|
Seeing as this is the first we (or I) have ever even heard of this bug, I think it is safe to assume that performance is a non-issue here (as long as it doesn't explode). It's even the double-updating that is the root cause, so it's basically an inefficiency introduced by the user. |
|
@syranide @jordwalke Batching innerHTML setting across multiple trees like we do now could be useful for things like petehunt's sortable component (https://gist.github.com/petehunt/7882164) where each child makes a separate component root. We're only running into trouble right now when combining that with multiple updates to the same component at one time. I agree though that there is code complexity and this might not be super common. |
|
After an initial look at the diff, either way, this structure seems much cleaner. Whether or not we batch markup generation across multiple parents/roots, queueing mutations is helpful for animations in the future (and sets us up to be more testable). Queueing of mutations in general isn't the main cause of code-complexity - batching markup across multiple parents/roots is. This diff doesn't remove the later, but makes the former more elegant/correct. Note how @spicyj eliminated this |
|
Unrelated to this diff: I would like it if the core were architected more like this - where mutations are queued. That allows us to clearly separate the reconciliation process into two separate, testable stages:
Right now, before and after this diff, child creation mutations are queued up, but other types of mutations are not (things like setting properties etc). If we make sure that every mutation is queued (assuming that it doesn't hurt performance in practice, not that I'm concerned) - then we can compute the diff, and then easily implement various strategies of carrying out the mutations over time. We could throttle across This diff nudges us in that direction by making queueing easier to reason about in general by tying it to the |
|
Side-note, we should probably add http://jsbin.com/caxipewo/6/edit (#1147 (comment)) or some variation of it as a test. |
|
@sebmarkbage Currently, when the reconcile transaction is closed, the mount-ready callbacks are executed, meaning that all DOM operations should have been flushed by that point. Like you suggest, perhaps we should change setState so that the transaction is executed later, but the current behavior is wrong. |
Should make facebook#1350 better and will also take away any performance hit from facebook#1157. Test Plan: grunt test
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@sebmarkbage, can you take another look at this before I pull it in |
|
Can we get a unit tests for the observable change in behavior? (There is one in the referenced issue.) This will increase memory usage for large updates. :/ Could another solution be to keep mounts in a hash map based on IDs so that you can clear them or replace them when there is a subsequent remove or update respectively? Is this even a valid behavior? componentWillUpdate shouldn't cause setState. Can we just forbid it? |
|
Sorry, the description here is a bit out of date. The referenced issue was actually fixed by #1363 and I already added a test case for it. I believe that this PR should actually cause no observable differences; it just cleans up the code. When does this increase memory usage? We can probably forbid componentWillUpdate from calling setState. It's already disallowed when calling setState on the component whose componentWillUpdate is running. I can do that as a separate diff. |
|
The underlying memory used by the multi-child queue can be reused by other updates in the same transaction. Difficult to predict real world behavior since it can depend on GC implementation and memory load. On a memory constrained device (like low-end Android) this could potentially crash the browser instead of GC:ing. |
|
It seems unlikely to me that the list of queued updates would be significant in comparison to the cost of the actual DOM nodes and component instances. Do you disagree that the queueing that ReactMultiChild does is odd and belongs in ReactReconcileTransaction? |
|
I agree this is cleaner. From what I can see, this would only increase the number of object allocations by ~3 per simultaneous component tree rendered. I don't think that is a concern. The reason I originally batched child mutation for the entire component tree (instead of only among siblings) was because I found that generating a bunch of nodes using a single As a follow-up, could we consider batching cross-hierarchy between transactions? |
|
@yungsters Not sure if you saw #1358 and #1363; now I believe we share a reconcile transaction in at least every case where MultiChild batched before, so we're still batching |
|
This goes in direct opposite direction of the live reconciler that @jordwalke and I started. That's something we should re-explore. I was concerned that every update would be batched (which is something I considered for better error handling). This only batches moves (which are rare) and deletions. A large set of deletions are common but the signature is pretty small. I think this is fine to pull in if we want to get it in. Not sure if there's a huge win since it's potentially slower and comes with a risk. @yungsters Do you think it would be worth while pulling this in to see if it fixes that weird reconciling bug we've seen? |
|
Interesting… that bug escapes me, but I can see how debugging across hierarchies might have been making it really hard for me to trace the root cause. Yes! Let's get this in. |
|
Assuming by "live reconciler" you mean not pre-flattening children (cf. #942), I don't believe this makes that any harder. I think this is the first I've heard of "that weird reconciling bug"; if it's still happening let me know how to repro and I'll take a look. |
Fixes facebook#1147. This now forces all DOM operations for a subtree to be applied when the calling `setState` or `React.renderComponent` call returns (when updates aren't being batched). This means that we can't batch innerHTML setting across different component hierarchies, but our strategy for doing so before seems flawed. It could be possible to make the old way work but it would require making setState always async even when batching isn't in play and refactoring DOMChildrenOperations to not be confused by multiple updates to the same node.
|
@spicyj That's the trouble, no one can consistently reproduce the issue. The symptom is that the Facebook comment user interface (e.g. below each post), the row containing the I traced it to |
|
@yungsters Today @sebmarkbage concluded it might be #1593. |
|
Is it valuable to keep this open? Do we have plans to move forward or is this going to end up just being an experiment that influences further designs? |
|
I still think this should be merged. Last I remember, @sebmarkbage thought it was probably fine but didn't want to merge it while you were all out on vacation. |
|
We should bring this in but it requires a lot of testing to make sure that we don't have functionality or performance regressions. This doesn't actually change any behavior. We can pull this in as part of something larger, such as error boundaries or something related like that, that actually fixes something. |
|
@spicyj looks like this needs an update before we can try to merge this. |
|
@spicyj Is this still happening?
Seems like that is the default behavior now, yes? |
|
@yaycmyk Might happen as part of some other upcoming work. Any reason it's important to you? |
|
@spicyj Nothing in particular. I was looking through some of the older PRs and this one leapt out as interesting since the |
|
This is just an internal refactoring, should have no user-facing changes. |
|
Superseded by #5547. Do I get a prize for closing an old PR? |
|
💝 |
|
🎁 |
|
🎈 🎉 🎁 |
Fixes #1147.
This now forces all DOM operations for a subtree to be applied when the calling
setStateorReact.renderComponentcall returns (when updates aren't being batched). This means that we can't batch innerHTML setting across different component hierarchies, but our strategy for doing so before seems flawed. It could be possible to make the old way work but it would require making setState always async even when batching isn't in play and refactoring DOMChildrenOperations to not be confused by multiple updates to the same node.Still need to add tests. Not sure this is exactly what we want to do but I think it agrees more with what we've been doing.