[SuperEditor] Fix typing lag in large documents (Resolves #751) #839
Conversation
|
I made an attempt to optimize mouse dragging: 2022-10-26.22-12-20.mp4But it doesn't help when selecting nodes at the end of the document. |
|
@matthew-carroll Right now, the only thing I think is missing is a way to detect when BTW. I replaced |
|
@Jei-sKappa @jmatth @Mijawel - Can each of you give this PR a try and verify that it resolves your performance problems? |
|
Looks good. No obvious lag at 500 nodes/50,000 words. At 1000 nodes/100,000 words there is a decent amount of lag but it is still usable. Much better than before. Was getting lag at 50 to 100 nodes. |
|
For me the performance is good, btw I also notice what @Mijawel said, but it's totally fine unless you type at 200+ wpm. Unfortunately while testing the editor I noticed a bug when pasting text, I want to make clear that it could be totally unrelated but I wanted to point it out.
Registrazione.schermo.2022-10-29.alle.14.38.28.movThe error output is: The relevant error-causing widget was: When the exception was thrown, this was the stack: |
|
@Jei-sKappa I tested on the latest main and the issue also happens, so it should be unrelated. |
|
@Jei-sKappa can you file a new issue for that copy bug? |
|
BTW @Mijawel and @Jei-sKappa - would you two mind telling us a bit about how you're using Super Editor? If you'd like to send that info privately, you're welcome to come over to the Super Editor Slack channel. I'm curious, because the more we know about community use-cases, the better we can focus our roadmap. |
Issue just created!
Of course, I’d be happy to share how I use the editor. |
super_editor/lib/src/default_editor/layout_single_column/_layout.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/layout_single_column/_layout.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/layout_single_column/_layout.dart
Outdated
Show resolved
Hide resolved
| if (midleIndex + 1 < _topToBottomComponentKeys.length) { | ||
| // Check the gap between two components. | ||
| final nextComponentBounds = _getComponentBoundsByIndex(midleIndex + 1); | ||
| if (dy > componentBounds.bottom && dy < nextComponentBounds.top) { |
There was a problem hiding this comment.
Shouldn't we divide these gaps in half? Otherwise, the component that happens to be in the middle of a search receives special treatment with regard to when it's considered selected.
| attribution, | ||
| range, | ||
|
|
||
| // During style phase, we check which components changed, so we only re-style those components. |
There was a problem hiding this comment.
Can you file an issue for this? We should probably have a way to compare content.
Also, please add a TODO to each of these comments that references the issue that you file.
There was a problem hiding this comment.
Maybe this will change in #748 ? I will reference this issue then.
There was a problem hiding this comment.
Nevermind, I was confused with another statement.
There was a problem hiding this comment.
We have a way to compare content. The AttributedText equality operator is defined as:
@override
bool operator ==(Object other) {
return identical(this, other) ||
other is AttributedText && runtimeType == other.runtimeType && text == other.text && spans == other.spans;
}If we just toggle the attributions in the same instance, the first condition will always be true.
There was a problem hiding this comment.
Can you reframe this as an action item? I'm not sure what to do with the above information...
There was a problem hiding this comment.
ParagraphComponentViewModel compares its AttributedText as part of its equality operator (text == other.text).
Before the change, we were only modifying the contents of the same AttributedText that was referenced in the viewmodel. The contents changed, but as it's referencing the same object, the equality operator returns true, as identical(this, other) is true.
There was a problem hiding this comment.
Ok. Let's change the comment to:
// Create a new AttributedText with updated attribution spans, so that the presentation system can
// see that we made a change, and re-renders the text in the document.
Let's change the implementation to:
node.text = AttributedText(
text: node.text.text,
spans: node.text.spans.copy()..addAttribution(
newAttribution: attribution,
start: range.start,
end: range.end,
),
);
| onViewModelChange: ({required addedComponents, required changedComponents, required removedComponents}) { | ||
| // The listener is called two times. The first one for the text change, | ||
| // and the second one for the selection change. | ||
| componentChangedCount = max(componentChangedCount, changedComponents.length); |
There was a problem hiding this comment.
It's not obvious to me what this max() is trying to accomplish. Can we check the behavior with hard-coded values to make things super clear?
There was a problem hiding this comment.
It's because typing a letter cause 2 calls to onViewModelChange, so the selection change overrides the value we care about. I will try to change to make it more obvious.
This might change after #748 is closed.
There was a problem hiding this comment.
Can you use an if-statement, or some explicit control structure, rather than calculate a max number?
| attribution, | ||
| range, | ||
|
|
||
| // During style phase, we check which components changed, so we only re-style those components. |
There was a problem hiding this comment.
Can you reframe this as an action item? I'm not sure what to do with the above information...
|
This resolves the performance issues for me. |
| @@ -162,7 +168,7 @@ class MutableDocument with ChangeNotifier implements Document { | |||
|
|
|||
| @override | |||
| int getNodeIndex(DocumentNode node) { | |||
| return _nodes.indexOf(node); | |||
| return _nodeIndicesById[node.id] ?? -1; | |||
There was a problem hiding this comment.
This is changing the behavior of the method slightly. It will now return an index if you pass a node that shares an id with one in the document even if they are otherwise unequal. Additionally, if someone implemented custom nodes with overridden == behavior, this method may now return -1 instead of a valid index.
In my implementation I used the cache to quickly look up the node and then performed the comparison to maintain the existing behavior. Code here.
There was a problem hiding this comment.
I updated to add the equality check.
There was a problem hiding this comment.
I thought about this a bit more. Why would there ever be 2 nodes with the same ID and different content? The point of the ID is to uniquely refer to a node within a document...
There was a problem hiding this comment.
@jmatth can you let me know when/why this change would be a problem?
There was a problem hiding this comment.
I could trivially construct two nodes with the same ID that are otherwise unequal, store one in a MutableDocument, then pass the second one to getNodeIndex and get a valid index back. I find that result unexpected. Best example I can think of for why this would happen is converting between node types, such as a paragraph to a list item or similar, but I don't actually have any real world examples. If you don't think that's a reasonable use case to accommodate then this method should just be removed entirely, because getNodeIndexById also exists, its signature makes it much clearer what the criteria for a match are, and without the equality check the two methods serve the exact same purpose.
There was a problem hiding this comment.
That said, if removal looks acceptable, there's probably no reason to make that breaking change now. We're trying to land a semi-stable release after this PR. So @angelosilvestre if you think removal looks OK, then add a "deprecated" annotation to the method and we'll merge it in as-is.
There was a problem hiding this comment.
I will take a look.
There was a problem hiding this comment.
@matthew-carroll We are using this method in a number of places, but we could change them to use getNodeIndexById. Should I add the deprecated annotation and change the usages?
There was a problem hiding this comment.
If you've inspected all of those locations, and you're comfortable that changing them won't be a big deal, then sure, you can go ahead and deprecate this method. Please be sure to include a deprecation message that points to the other method.
There was a problem hiding this comment.
I marked getNodeIndex as deprecated and changed the places which use this method.
| } | ||
|
|
||
| if (_nodes[index] != node) { | ||
| // We found a node by id, but the contents aren't equal. |
There was a problem hiding this comment.
I think I would word this a bit differently: // We found a node by id, but it wasn't the node we expected. Therefore, we couldn't find the requested node
[SuperEditor] Fix typing lag in large documents. Resolves #751
There is a lag when typing in a document with hundreds of nodes.
This PR is based on the investigation of @jmatth and @Jei-sKappa.
Some analysis after testing with the Frankenstein document posted by @jmatth (added as a gist here):
getNodeIndextakes some hundred microseconds to complete. It basically walks the entire list of nodes doing a deep comparison. Typing a single letter causesgetNodeIndexto be called more than 3000 times. This function is called bygetNodeBefore,getNodeAfterand others.getNodeByIdtakes some microseconds, but far less thangetNodeIndex.ParagraphComponentViewModel.==is broken. CheckingtextStyleBuilder == other.textStyleBuilderalways evaluates tofalse. Typing a single letter in a paragraph causes all the components to be considered changed.This PR changes
MutableDocument, adding aMapto quickly lookup a node index by its id. ThisMapis eagerly updated when the document is created and in each operation which affects the node indices. AMapto lookup a node by its id was also added. Because of thisMap's, I had to changenodesto return as an unmodifiable List, to avoid the node list being out of sync with the node maps.This PR also removes
textStyleBuilderfrom the==operators of the view models. However, we still need to figure out how to handle the case in whichtextStyleBuilderdo change.After this, I experienced another issue: toggling an attribution wasn't causing the style to be updated immediately. This happens because toggling an attribution just changes
AttributedTextinternally. When the viewmodel checks if the text changed, it is the same instance, so it's considered as not changed. This bug was hidden because comparingtextStyleBuilderalways causes the viewmodel to be considered as changed.This PR changes the attribution commands to create new
AttributedText's.After these changes:
2022-10-26.21-27-48.mp4
Dragging to select has still some lag:
2022-10-26.21-34-12.mp4
Most of the time seems to be spent on
getDocumentSelectionInRegion, more specifically in a call tolocalToGlobalat:super_editor/super_editor/lib/src/default_editor/layout_single_column/_layout.dart
Lines 418 to 429 in d5e7460
When testing with this document I found another bug that is present in the latest main. Typing
1.to create a list item it throws this exception:type 'ListItemNode' is not a subtype of type 'ParagraphNode' of 'element'. This exception doesn't happen with the default example document.