fix: dispose performance#6894
Conversation
rachel-fenichel
left a comment
rachel-fenichel
left a comment
There was a problem hiding this comment.
Overall looks good. I have some questions about how certain changes preserve old behaviour, but I'll try to find you to discuss in-person.
| if (this.isDeadOrDying()) return; | ||
|
|
||
| if (this.onchangeWrapper_) { | ||
| this.workspace.removeChangeListener(this.onchangeWrapper_); |
There was a problem hiding this comment.
Will the existing change listener trigger on the call to unplug on line 318?
There was a problem hiding this comment.
Added the removal to dispose as well, with explanatory comment.
| for (let i = 0, input; input = this.inputList[i]; i++) { | ||
| input.dispose(); | ||
| } | ||
| [...this.childBlocks_].forEach((c) => c.disposeInternal()); |
There was a problem hiding this comment.
This used to use the original array and go backwards. Now it uses a copy and goes forward. Why the change?
There was a problem hiding this comment.
So we discussed yesterday, and I said the issue was that setParent(null) updates the childBlocks_ of the parent block. So calling dispose on children would mutate the array we were looping over. We could either deal with this by looping backwards (as previous) or by looping over a copy that doesn't get mutated.
However! I checked it again after we chatted and since we're no longer calling unplug on every block, we're also not calling setParent(null) on every block. So the array isn't being mutated at all, and we can just loop over it directly =)
BeksOmega
requested review from
rachel-fenichel
and removed request for
NeilFraser
3 participants
The basics
npm run formatandnpm run lintThe details
Resolves
Fixes #6890
Proposed Changes
Improves performance of deleting blocks.
See #6890 for previous performance. The new performance is ~300ms total, and ~100ms for
checkAndDelete.Reason for Changes
Speeeed
Test Coverage
Manual testing + fixing failing tests.
Added one test to check that blocks do not receive their own delete events.
Documentation
N/A
Additional Information
We save an extra 20ms after #6876 is merged.
Best to review this commit-wise since there is some code reorganization.