flutter-quill icon indicating copy to clipboard operation
flutter-quill copied to clipboard

Continous jank in text with 1k rows

Open vargab95 opened this issue 1 year ago • 18 comments

Is there an existing issue for this?

Flutter Quill version

master branch

Steps to reproduce

  1. Clone the repo
  2. Go to the example folder
  3. Start the example
  4. Open the performance tab of flutter dev tools
  5. Delete the example text
  6. Paste example + enter 1k times
  7. Click in a row and add one character

Expected results

A document with few thousand rows renders at least with 30 FPS when edited.

Actual results

I've tested with a linux desktop PC (AMD Ryzen 5 5600G). With 1k rows, it rendered in 30-60ms, with 2k rows in 60-80ms, with 4k rows in 100-120ms, with 8k it took 200-250ms. It is more or less exactly O(n). Which means that even when a user edits a document with 1k rows, it will generate a jank frame.

Additional Context

I think, having documents with 1k rows are not so rare in case of a rich text editor.

I've searched through the issues and I've found that in https://github.com/singerdmx/flutter-quill/issues/997 it was already described. However, on master I cannot see the rasterization charts, but jank frames are still problematic.

vargab95 avatar Nov 12 '24 09:11 vargab95

master branch

Have you tried older/other major versions of the project? If yes, can you mention them to ensure if this is a regression.

EchoEllet avatar Nov 12 '24 09:11 EchoEllet

In my project where I've first detected the jank frames, I'm using 10.8.5. I'll do the same tests with some older versions as well.

vargab95 avatar Nov 12 '24 09:11 vargab95

It's the same with v9.6.0. The only difference is that flutter devtools have also reported 1 shader compilation jank. I haven't seen it in the latest version.

vargab95 avatar Nov 12 '24 09:11 vargab95

I've started a pixel 5 emulator and using that even editing the original example document generates jank frames. When I hit a key on the virtual keyboard, it results in a jank frame with 40-70ms duration.

vargab95 avatar Nov 12 '24 09:11 vargab95

some older versions

Can you also test the latest pre-release? If the migration is too much to test, maybe with the example of the flutter_quill?

EchoEllet avatar Nov 12 '24 09:11 EchoEllet

Sure, with the flutter_quill example it's still the same 30-60ms with 1k rows when using v11.0.0-dev.12.

vargab95 avatar Nov 12 '24 10:11 vargab95

I've enabled tracking widget builds in all code. The result is that a single render of QuillRawEditorMultiChildRenderObject takes 59.5ms.

60ms_render

And another screenshot, where you can see one of the 1k TextLine objects.

60ms_render_zoomed_in

vargab95 avatar Nov 12 '24 10:11 vargab95

Another strange thing. Can it be that the toolbar is also re-rendered on change? I did some more measurements with tracking widget builds in all code and it seems like it takes a measurable proportion of the build time. I guess it's due to the fact that they use the same controller, so when the notifyListeners function is called, then both are rebuilt.

toolbar-rerender

vargab95 avatar Nov 12 '24 11:11 vargab95

Just an idea, would it be possible to add a Widget cache into the QuillRawEditor? I've simply added a map to the _buildChildren method of QuillRawEditorState which stores the widget by the node.hashCode and only rebuilds it when something has changed in that position. It reduced the build time from 100-120ms to 20-30ms in case of 4k rows and from 200-250ms to 30-40ms in case of 8k rows.

You can see my changes below. It's just a hack in it's current state, but if nodes can have a unique id, like an integer, incremented on each node creation, then it may be doable.

What is your opinion? The problem is that I don't know the flutter_quill code base, so maybe there is a blocker with this approach.

diff --git a/lib/src/controller/quill_controller.dart b/lib/src/controller/quill_controller.dart
index 30c16eae..fd5ed71f 100644
--- a/lib/src/controller/quill_controller.dart
+++ b/lib/src/controller/quill_controller.dart
@@ -264,6 +264,8 @@ class QuillController extends ChangeNotifier {
         const TextSelection.collapsed(offset: 0));
   }
 
+  int? changePosition;
+
   void replaceText(
     int index,
     int len,
@@ -278,6 +280,8 @@ class QuillController extends ChangeNotifier {
       return;
     }
 
+    changePosition = index;
+
     Delta? delta;
     Style? style;
     if (len > 0 || data is! String || data.isNotEmpty) {
diff --git a/lib/src/editor/raw_editor/raw_editor_state.dart b/lib/src/editor/raw_editor/raw_editor_state.dart
index b3c49ddc..7f351223 100644
--- a/lib/src/editor/raw_editor/raw_editor_state.dart
+++ b/lib/src/editor/raw_editor/raw_editor_state.dart
@@ -576,6 +576,8 @@ class QuillRawEditorState extends EditorState
     }
   }
 
+  final Map<int, Directionality> _lines = {};
+
   List<Widget> _buildChildren(Document doc, BuildContext context) {
     final result = <Widget>[];
     final indentLevelCounts = <int, int>{};
@@ -598,9 +600,20 @@ class QuillRawEditorState extends EditorState
       prevNodeOl = attrs[Attribute.list.key] == Attribute.ol;
       final nodeTextDirection = getDirectionOfNode(node, _textDirection);
       if (node is Line) {
-        final editableTextLine = _getEditableTextLineFromNode(node, context);
-        result.add(Directionality(
-            textDirection: nodeTextDirection, child: editableTextLine));
+        if (controller.changePosition != null &&
+            _lines[node.hashCode] != null &&
+            !node.containsOffset(controller.changePosition!)) {
+          result.add(_lines[node.hashCode]!);
+        } else {
+          final editableTextLine = Directionality(
+            textDirection: nodeTextDirection,
+            child: _getEditableTextLineFromNode(node, context),
+          );
+
+          _lines[node.hashCode] = editableTextLine;
+
+          result.add(editableTextLine);
+        }
       } else if (node is Block) {
         final editableTextBlock = EditableTextBlock(
           block: node,
@@ -632,6 +645,7 @@ class QuillRawEditorState extends EditorState
           customLinkPrefixes: widget.config.customLinkPrefixes,
           composingRange: composingRange.value,
         );
+
         result.add(
           Directionality(

vargab95 avatar Nov 12 '24 11:11 vargab95

Could you try to create a PR with these changes?

CatHood0 avatar Jan 26 '25 05:01 CatHood0

I tested the changes, but, in some cases can cause unexpected behaviors (like, not updated selection rects in different nodes, weird selection rects appears when we are not selecting nothing, etc)

CatHood0 avatar Jan 26 '25 05:01 CatHood0

As I wrote in the previous comment, in its current form it is just a hack (tested only for this single performance measurement). I implemented it to check whether widget caching could solve the performance issue and added as a comment to start a discussion whether this approach makes any sense according to devs having more experience with flutter-quill than I. As you can see from the previous comment, it helped a lot in this specific case, but still could not solve the problem. I plan to get to know the code base better when I'll have more free time and try to come up with a more mature & feasible solution.

vargab95 avatar Jan 27 '25 13:01 vargab95

Hi @vargab95, could you try using version 11.2.0 to see if the situation is improved?

EchoEllet avatar Mar 26 '25 07:03 EchoEllet

It's the same. Maybe a bit better. I retested it with the 8k rows scenario with version 11.2.0 and with 10.8.5. Both shows 200-250 ms junk on each keystroke. Maybe the average is a bit lower in 11.2.0, but it's still in this range.

vargab95 avatar Mar 26 '25 08:03 vargab95

It's more a matter of node rendering. Simply put, since everything is rendered, performance is affected. Instead of caching nodes, we should change the current editor implementation to avoid rendering nodes that aren't on the screen.

CatHood0 avatar Mar 26 '25 09:03 CatHood0

@vargab95 I don't know if this can help you, but, check this line. Here is the place where we need to apply this fix (defaultPaint is a method that is painting all children without checking if them are into the viewport). I'll check it after finish my last PR.

CatHood0 avatar Mar 26 '25 19:03 CatHood0

I just did a test with 8k lines using 11.2.0 where I commented this line out. Of course, it did not paint anything, but I could just replace everything by using keyboard shortcuts. The test steps were

  1. Checkout 11.2.0
  2. Comment out defaultPaint call what you linked
  3. Run it with flutter run -d linux
  4. Click in the editor
  5. Ctrl+A, then delete
  6. Ctrl+V 8k lines of "example"
  7. Click into the editor at a random place and push letter 'a' multiple times

During step 7, the performance view shows 190-220ms junks. So it's faster by around 5-10%, but not much better.

vargab95 avatar Mar 28 '25 08:03 vargab95

This means that we should not only focus on node rendering, but we should also find the other parts that may be degrading performance.

CatHood0 avatar Mar 28 '25 16:03 CatHood0