optimize isParentRemoved for large remove lists #1368
Conversation
🦋 Changeset detectedLatest commit: c68b3f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -688,6 +690,7 @@ export default class MutationBuffer { | |||
| ? true | |||
| : undefined, | |||
| }); | |||
| this.removesMap.set(nodeId, this.removes.length - 1); | |||
There was a problem hiding this comment.
I don't understand the rhs of this set here; it doesn't seem significant.
Should the data structure be a Set instead of a Map?
There was a problem hiding this comment.
If that's true, then rather than adding another variable that needs to be maintained, we could convert the this.removes type into a Map indexed by nodeId new Map<number, removedNodeMutation>();
and then when the payload is emitted, convert to the expected array:
-removes: this.removes
+removes: Array.from(this.removes.values())
There was a problem hiding this comment.
yeah the map was unnecessary before, changed removes to just be a map of the id to the mutation as you suggested
ff209e5 to
6855186
Compare
6855186 to
c68b3f2
Compare
|
I think this has been superseded by #1543 which no longer does the expensive |
Encountered a mutation that removes ~1400 nodes. Noticed a lot of the processing time was tied up in
isParentRemoved, which repeatedly does an iterative search of theremovesarray. I added a map (nodeId => index in removes array) to speed up this process. See before/after images below:Before:
After:
I didn't see a difference in the existing benchmarks, even though one of them involves removing a lot of nodes. The problem functionality shown in the profiles above involved scrolling through a large table that would dynamically remove/add pages of data as you scroll.