Improve tldraw example performance #640

devleejb · 2023-09-03T14:55:25Z

What this PR does / why we need it?

In the tldraw example project, there's long latency when moving large shapes (e.g., DrawShape).
The reason is that the unchanged data (e.g., drawing points in DrawShape) is sent when moving shapes. So, in this PR, only point data (a point of shape on the whiteboard) is sent to the Yorkie server.

Any background context you want to provide?

I was inspired by this solution.
codepair issue #194

What are the relevant tickets?

Fixes #

Checklist

Added relevant tests or not required
Didn't break anything

CLAassistant · 2023-09-03T14:55:34Z

All committers have signed the CLA.

codecov · 2023-09-03T16:00:35Z

Codecov Report

Patch coverage has no change and project coverage change: -0.06% ⚠️

Comparison is base (cb4551b) 88.63% compared to head (30abe8f) 88.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #640      +/-   ##
==========================================
- Coverage   88.63%   88.57%   -0.06%     
==========================================
  Files          80       80              
  Lines        8894     8894              
  Branches      816      816              
==========================================
- Hits         7883     7878       -5     
- Misses        704      708       +4     
- Partials      307      308       +1

see 1 file with indirect coverage changes

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

krapie

Thank you for your contribution!

This was one of the highest-priority issues that Yorkie's tldraw example had.
It seems like this reduces PushPull() API's data size by several times.

I have one thing to discuss with you.

How do you think of expanding this idea to only update the shape's changes (coordinate change, style change, size change, etc) by comparing the previous shape and the updated shape?

devleejb · 2023-09-04T07:24:24Z

Thank you for your comment!

Before PR, I considered about expanding this idea to only update the shape's changes. To compare difference of two object, I think it's better to use library(e.g., Lodash, deep-object-diff, ...) than implement detecting deep difference(because of complexity). And to emphasize yorkie-js-sdk's usage, I think it is appropriate not to use other libraries at most.

If you think it's okay to use a library, I'll implement it. Or, is there any idea?

krapie · 2023-09-04T13:40:29Z

@devleejb I think it will be okay to show usage of other libraries because this will show other developers that they can use this approach to optimize the size of document updates.

How do you think of this idea, @chacha912?

devleejb · 2023-09-05T00:27:03Z

@krapie
OK, I'll implement it.

I wonder what you think about this.
How about change update function in yorkie-js-sdk to ignore unchanged data?

hackerwins

Thank you for your contribution.

I understand that this PR has made some changes to use CRDT to transmit data every time a shape is moved.

I have two questions:

Can Presence be used in the tldraw example to represent the state that needs to be sent to peers who are still editing but have not completed their edits?

Additionally, is there a way to check how much improvement has been made?

krapie · 2023-09-05T06:54:52Z

How about change update function in yorkie-js-sdk to ignore unchanged data?

@devleejb It seems like a good idea, but it may be out of scope.

And like @hackerwins mentioned, it will be good to measure performance improvement.

devleejb · 2023-09-05T14:35:18Z

@hackerwins

In previous tldraw example, the data is transmitted every time as same as my PR. But, in the example, when moving shapes, the unchanged data in shape object(e.g., color, point, ...) is transmitted and it made long latency. So, I send only changed data when moving shapes.

Can Presence be used in the tldraw example to represent the state that needs to be sent to peers who are still editing but have not completed their edits?

I understand that your question is that whether Presense can be used in representing editing state(e.g., moving, changing size, ...)

Now, when moving shapes, the below information is included in doc.presence

[
    {
        "clientID": "64f73119763112ce4175cff7",
        "presence": {
            "tdUser": {
                "id": "0ff70062-c49b-4af7-38a2-1cb14a9a671d",
                "color": "#07968a",
                "point": [
                    466.64,
                    671.94
                ],
                "selectedIds": [
                    "fe0bcde5-009a-4927-1bae-0a3d7cec5130",
                    "bf979117-c4ce-4a91-3cb8-610d708e11a2",
                    "ec5a7d38-3364-432a-089c-06c246a61f15"
                ],
                "activeShapes": [],
                "status": "connected",
                "metadata": {
                    "name": "Anna-diane"
                },
                "session": true
            }
        }
    }
]

There's selectedIds field. But, the data can't distinguish whether moving or changing shapes.

I also can add new field to Presence. But, detecting user's editing state can be calculated in onChangePage function.

Is there a way to check how much improvement has been made?

I think that it can be achieved using comparison of latency.

For test, I moved drawing shape.

Previous PushPullChanges Response Time

227ms

283ms

326ms

...

Previous PushPullChanges Response Time

15ms

15ms

16ms

...

The latency is decreased. And the decreasing rate is different for shape size.
The screenshot is in this URL

hackerwins · 2023-09-06T01:38:18Z

For sharing purposes, the information @devleejb left in the comment is also left here.

Example:

Before:

After:

Jaeheon-Sim · 2023-09-06T06:35:44Z

@devleejb
This improvement is truly what I wanted, Thank you!
But, I found an error in this PR

In this code, the Tldraw deletion element is not reflected in Yorkie.

How about applying this code?

Before:

 if (isMoving) {
            root.shapes[id].point = shape?.point;
            return;
          }

After:

if (isMoving && shape?.point) {
            root.shapes[id].point = shape?.point;
            return;
          }

OR

Deleted thing's shape is null when OnChangePage is working .
So, how about detect it on first step?
like...

 doc.update((root) => {

           if (!shape) { // delete element
                delete root.shapes[id];
                return;
           }
          const rootShapePoint = root.shapes[id]?.toJS?.().point;
          const isMoving =
            root.shapes[id] &&
            JSON.stringify(rootShapePoint) !== JSON.stringify(shape?.point);

          if (isMoving) { // move element
            root.shapes[id].point = shape?.point as Array<number>;
            return;
          }

          // create element
           root.shapes[id] = shape;
          
        });

This can detect deleted elements on Tldraw

Thank you!!

devleejb · 2023-09-06T07:31:20Z

@Jaeheon-Sim
Thank you for your comment.

I added code for deleting shape.

Thank for your help.

krapie

@devleejb The latency improvement will become larger as we move more shapes simultaneously. (When I measured latency improvement by moving 20 shapes by 1 pixel, latency improved 35ms to 8ms).

How do you think of expanding this idea to only update the shape's changes (coordinate change, style change, size change, etc) by comparing the previous shape and the updated shape?

After you implement this idea, please let me know.

devleejb · 2023-09-07T16:27:05Z

@krapie

I implemented your idea by comparing changed property in object.

Thank you.

krapie

Thank you for implementing this idea!
It will be good to test performance of this idea with previous ones.

Also, I left a small comment below.

examples/react-tldraw/src/hooks/useMultiplayerState.ts

devleejb · 2023-09-08T13:10:03Z

@krapie

I removed it. Thank you!

krapie

Thank you for your contribution.
Looks good to me 👍🏼

hackerwins

Thanks for your contribution.

devleejb added 2 commits September 3, 2023 23:43

feat: Update YorkieDocType

675b118

Improve performance when user moves shapes

a3be3f0

krapie requested review from krapie and chacha912 September 3, 2023 15:27

krapie assigned devleejb Sep 3, 2023

krapie reviewed Sep 4, 2023

View reviewed changes

devleejb requested a review from krapie September 4, 2023 07:46

hackerwins self-requested a review September 5, 2023 00:47

hackerwins reviewed Sep 5, 2023

View reviewed changes

hackerwins force-pushed the main branch from 6272388 to f30ce88 Compare September 5, 2023 10:42

devleejb requested a review from hackerwins September 6, 2023 01:20

feat: Add exception handling for deleting shape

a0f3389

krapie reviewed Sep 7, 2023

View reviewed changes

devleejb added 3 commits September 8, 2023 01:20

feat: Optimize onChangePage callback

6b49388

Merge branch 'main' into tldraw-performance

6d5f76d

Update package-lock.json

8dd5a47

devleejb requested a review from krapie September 7, 2023 16:27

krapie requested changes Sep 8, 2023

View reviewed changes

examples/react-tldraw/src/hooks/useMultiplayerState.ts Show resolved Hide resolved

chore: Remove duplciated code

30abe8f

devleejb requested a review from krapie September 8, 2023 13:09

krapie approved these changes Sep 11, 2023

View reviewed changes

hackerwins approved these changes Sep 12, 2023

View reviewed changes

hackerwins merged commit 876d3dc into yorkie-team:main Sep 12, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve tldraw example performance #640

Improve tldraw example performance #640

devleejb commented Sep 3, 2023

CLAassistant commented Sep 3, 2023 •

edited

codecov bot commented Sep 3, 2023 •

edited

krapie left a comment •

edited

devleejb commented Sep 4, 2023 •

edited

krapie commented Sep 4, 2023

devleejb commented Sep 5, 2023

hackerwins left a comment

krapie commented Sep 5, 2023

devleejb commented Sep 5, 2023 •

edited

hackerwins commented Sep 6, 2023

Jaeheon-Sim commented Sep 6, 2023 •

edited

devleejb commented Sep 6, 2023

krapie left a comment

devleejb commented Sep 7, 2023 •

edited

krapie left a comment

devleejb commented Sep 8, 2023

krapie left a comment •

edited

hackerwins left a comment

Improve tldraw example performance #640

Improve tldraw example performance #640

Conversation

devleejb commented Sep 3, 2023

What this PR does / why we need it?

Any background context you want to provide?

What are the relevant tickets?

Checklist

CLAassistant commented Sep 3, 2023 • edited

codecov bot commented Sep 3, 2023 • edited

Codecov Report

krapie left a comment • edited

Choose a reason for hiding this comment

devleejb commented Sep 4, 2023 • edited

krapie commented Sep 4, 2023

devleejb commented Sep 5, 2023

hackerwins left a comment

Choose a reason for hiding this comment

krapie commented Sep 5, 2023

devleejb commented Sep 5, 2023 • edited

hackerwins commented Sep 6, 2023

Jaeheon-Sim commented Sep 6, 2023 • edited

devleejb commented Sep 6, 2023

krapie left a comment

Choose a reason for hiding this comment

devleejb commented Sep 7, 2023 • edited

krapie left a comment

Choose a reason for hiding this comment

devleejb commented Sep 8, 2023

krapie left a comment • edited

Choose a reason for hiding this comment

hackerwins left a comment

Choose a reason for hiding this comment

CLAassistant commented Sep 3, 2023 •

edited

codecov bot commented Sep 3, 2023 •

edited

krapie left a comment •

edited

devleejb commented Sep 4, 2023 •

edited

devleejb commented Sep 5, 2023 •

edited

Jaeheon-Sim commented Sep 6, 2023 •

edited

devleejb commented Sep 7, 2023 •

edited

krapie left a comment •

edited