Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache offset and length in document nodes #408

Merged
merged 2 commits into from
Sep 2, 2024

Conversation

Amir-P
Copy link
Member

@Amir-P Amir-P commented Aug 18, 2024

Fixes #244

Performance comparison with master ran locally on an M1 Mac:

Analyzing tests for scrolling
Average Frame Build Time: Target: 87.48 Reference: 87.62
90th Percentile Frame Build Time: Target: 96.96 Reference: 96.93
90th Percentile Frame Build Time: Target: 98.92 Reference: 99.47
Average GPU Frame Time: Target: 0.00 Reference: 0.00
Performance tests found no regression


Analyzing tests for editing
Average Frame Build Time: Target: 129.21 Reference: 762.61
90th Percentile Frame Build Time: Target: 135.17 Reference: 769.02
90th Percentile Frame Build Time: Target: 137.28 Reference: 770.56
Average GPU Frame Time: Target: 0.00 Reference: 0.00
Performance tests found no regression

@Amir-P Amir-P force-pushed the perf/cache_offset_and_length_for_nodes branch from 7a070fe to 30d0ca1 Compare August 20, 2024 19:05
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 94.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.04%. Comparing base (e725636) to head (c4a937b).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
...ackages/parchment/lib/src/document/attributes.dart 33.33% 2 Missing ⚠️
packages/parchment/lib/src/document/embeds.dart 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #408      +/-   ##
==========================================
+ Coverage   87.94%   88.04%   +0.09%     
==========================================
  Files          62       62              
  Lines       10364    10419      +55     
==========================================
+ Hits         9115     9173      +58     
+ Misses       1249     1246       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Amir-P Amir-P marked this pull request as ready for review August 20, 2024 19:15
@Amir-P Amir-P requested a review from amantoux August 20, 2024 19:26
@Amir-P
Copy link
Member Author

Amir-P commented Aug 20, 2024

This change is supposed to improve the performance of editor specially when editing and having cursor. I was hoping you guys can take a look, test things out, and give feedback on it. Many thanks in advance. @maelchiotti @simonbengtsson

@simonbengtsson
Copy link
Contributor

Sure! Tried now both on master and on the perf branch both with and without 100k characters. In summary I didn't really observe any differences. The 100k characters did not seem to matter and the experience was the same both in master and in the perf branch. The editing experience is almost lag free when writing normally so tried spamming the keyboard as fast as I could instead and then some extreme lag is observable. Could very much be related to my persistence code however and not related to fleather editor.

Attaching a video of that lag when keyboard spamming for reference. Here I have 100k characters in the editor and am using the perf branch but was about the same no matter what. Note that I stopped spamming the keyboard when I saw a visible character at the | line so all characters added after that is added by lag ie when I'm not touching the keyboard.

pref_branch_100k_characters.mov

@Amir-P
Copy link
Member Author

Amir-P commented Aug 21, 2024

Thank you @simonbengtsson for testing it. It's weird that you're not facing any issues with the master maybe your computer is too powerful! According to our benchmark tests we've seen ~3x improvement in average frame build time on macOS (from ~450ms to ~130ms for a frame).
BTW, what a nice looking app you have there!

@amantoux
Copy link
Member

Sure! Tried now both on master and on the perf branch both with and without 100k characters. In summary I didn't really observe any differences. The 100k characters did not seem to matter and the experience was the same both in master and in the perf branch. The editing experience is almost lag free when writing normally so tried spamming the keyboard as fast as I could instead and then some extreme lag is observable. Could very much be related to my persistence code however and not related to fleather editor.

The number of paragraphs (LineNode or BlockNode) makes a huge difference.
If you have 1000+ paragraphs things get pretty sticky

@Amir-P Amir-P force-pushed the perf/cache_offset_and_length_for_nodes branch from 30d0ca1 to c4a937b Compare August 23, 2024 10:05
Copy link
Member

@amantoux amantoux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Awesome performance comming in :)

@@ -254,7 +253,7 @@
}

@override
int get hashCode => hash3(key, scope, value);
int get hashCode => Object.hash(key, scope, value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this but it would have been nice to have it in another PR :)

@@ -155,7 +155,6 @@ final class LineNode extends ContainerNode<LeafNode> with StyledNode {
@override
LeafNode get defaultChild => TextNode();

// TODO: should be able to cache length and invalidate on any child-related operation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@amantoux amantoux merged commit bb6b948 into master Sep 2, 2024
4 checks passed
@amantoux amantoux deleted the perf/cache_offset_and_length_for_nodes branch September 2, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow typing performance when approx 45k characters used within editor
3 participants