Basic mutable batching with minimal overhead.#6050
Conversation
🦋 Changeset detectedLatest commit: dd1d5dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
Thanks for putting this together. This PR was useful, and I pulled a couple of ideas from it into #6039. The main thing I took from this is that the simple child-array rewrite helpers are a real win. Replacing spread-heavy I also added benchmark lanes based on this PR’s motivating case:
This PR is very strong for flat repeated On my machine:
But this PR does not solve the broader structural batching problem:
So I think this PR proves two things:
The part I’m still uncomfortable with is the semantic contract. A couple concrete code concerns:
My preference would be:
|
|
using a modified-to-fit version of #6039's
(*control group speedups are from a change to avoid spread operations in modify functions as mentioned by LLM above, which could land with or without batches) Code the same but for const applyBatchShim = (editor, ops) => {
batchDirtyPaths(
editor,
() => {
Editor.whileMutablyBatching(editor, () => {
for (const op of ops) {
editor.apply(op)
}
})
},
() => {
let dirtyPaths = []
for (const op of ops) {
if (Path.operationCanTransformPath(op)) {
dirtyPaths = dirtyPaths
.map(p => Path.transform(p, op))
.filter(p => p !== null)
}
dirtyPaths.push(...editor.getDirtyPaths(op))
}
updateDirtyPaths(editor, dirtyPaths)
}
)
} |
|
renamed |
|
drafting until I have time to review the API and write some proper docs |
Description
As described well in linked issue, large groups of tree updates make immutable changes in series which results in many more clone-and-tweak operations than are ever needed.
This PR solves this by providing opt-in stretches of mutability. During a Mutation Batch, only the first edit to a element's children will clone the element (so any pre-batch references remain immutable) and any further edits will just mutate in place. This means that the node tree is never proxied or out-of-date, it will always reflect the true state. (⚠️ Having mutable nodes in the tree can have other issues, see Limitations below)
At the end of a batch there is no need to apply any kind of draft, elements simply remain in place and are henceforth treated as immutable. "Finalization" is effectively free.
We add two methods to the editor object:
asMutationBatchandisBatchingMutationsthat work similarly towithoutNormalizingandisNormalizingrespectively.Issue
resolves #6038
Usage
Implementation
This is implemented by altering the modify functions that handle making immutable changes to the tree to instead make mutable changes when appropriate. Because batches are encapsulated in a single callback, this means the user API remains basically the same for anyone not using batching or using batching invisibly (within plugins/builtin transforms/etc)
Mutability is tracked with a simple
Set, which tracks children arrays that have been created during the batch and thus can be altered. (we track the children arrays themselves because using aset_nodeop on a node will result in a new node that shares the same children array object with the original)This adds 1-2 WeakMap lookups to every operation application to check whether the editor is in a batch, but otherwise does not affect any codepaths that don't involve mutation batching.
Uses
Currently used in
Editor#insertTextData--which is called when pasting--to speed up the split-and-insert operations per line.Limitations
Only children array modifications are tracked this way, changing the non-children properties of a node using
insert_text,remove_text, orset_nodenever mutates a node, only its ancestors. This is by design: multiple modifications to the same node in one frame seems like a problem that can be tackled through other means if not avoided altogether.Mutable elements in the node tree
As warned above elements and children arrays are liable to be mutable during a batch and should be treated as such. There's a few ways this could happen:
slate-historytracks operations onEditor#applyand thus--throughinsert_nodeandremove_node--tracks node references. This is actually fine because inserted nodes aren't added to the mutable set until after the first time a child is changed, and removed nodes are no longer in the tree to be changed, BUT, if a mutable removed node were added somewhere else later in the same batch it would be still be treated as mutable and thus mutate the operation in history.move_nodeslate-reactso it should never happen anyways.applyor directly accessing the un-overloaded versions of methods within built-in batches but that seems like an antipattern to me.It wouldn't be too hard to have a function that snapshots/commits a node--removing it from the mutable set mid-batch so it can be treated as immutable once more, (impl note: also traverse children to remove them) but I'll keep the API surface simple until there's demand for it
The more guardrails we add the more performance is sacrificed so I think its important to be judicial. Wrapping
Editor#childrenwith a getter, eg, would lead to a lot of de-opsAll-in-all, there's already something to consider when wrapping editor functions: Your operation may or may not result in any number of normalizations, so I dont think this kind of caveat is so unfamiliar to users as to cause issues.
Checks
yarn test.yarn lint. (Fix errors withyarn fix.)yarn start.)yarn changeset add.)