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

fix: paste large content with operational ui #7796

Merged

Conversation

Saul-Mirone
Copy link
Collaborator

@Saul-Mirone Saul-Mirone commented Aug 1, 2024

Screen.Recording.2024-08-01.at.10.55.25.mov

Copy link

vercel bot commented Aug 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
blocksuite ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 1, 2024 4:32am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
blocksuite-docs ⬜️ Ignored (Inspect) Visit Preview Aug 1, 2024 4:32am

Copy link

graphite-app bot commented Aug 1, 2024

Your org has enabled the Graphite merge queue for merging into master

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@Saul-Mirone Saul-Mirone marked this pull request as ready for review August 1, 2024 03:39
Copy link
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Saul-Mirone and the rest of your teammates on Graphite Graphite

@Saul-Mirone Saul-Mirone force-pushed the 08-01-fix_paste_large_content_with_operational_ui branch from 271983c to 2063e22 Compare August 1, 2024 03:39
@Saul-Mirone Saul-Mirone merged commit d836a06 into master Aug 1, 2024
19 checks passed
@Saul-Mirone Saul-Mirone deleted the 08-01-fix_paste_large_content_with_operational_ui branch August 1, 2024 04:52
@@ -470,6 +470,11 @@ export class Job {
children,
});

const nextTick =
typeof window !== 'undefined'
? window.requestAnimationFrame
Copy link
Member

@doodlewind doodlewind Sep 21, 2024

Choose a reason for hiding this comment

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

This is considered too slow in #8380. A brief perf test could be found in

#8427 (comment)

The non-blocking pasting could be MUCH slower.

Try this test file with ~1000 lines, the sync pasting completes immediately but async pasting takes ~10s.

clipboard-test.md

I think the downside switching to sync pasting may be about image importing. Not sure if we can make some improvement on the strategy.

doodlewind added a commit that referenced this pull request Oct 14, 2024
Close #8380

## Problem

The original process of converting snapshot JSON to blocks was significantly slowed to ensure UI responsiveness. It involved two types of async computations:

* Asynchronously converting snapshots to draft models.
* Asynchronously inserting draft models into the block tree using `doc.addBlock`.

Original import sequence:

```
Convert snapshot node A
Insert block A
Wait for tick

Convert snapshot node B
Insert block B
Wait for tick

Convert snapshot node C
Insert block C
Wait for tick
...
```

Despite `doc.addBlock` being synchronous, a `requestAnimationFrame` interval was added after each block insertion to prevent UI blocking. This resulted in long processing times when pasting thousands of blocks:

https://github.com/user-attachments/assets/3a6379c9-621e-490e-a79f-4377494d5915

## New Design

This PR reimplements the process:

1. Convert all snapshot nodes first, either using `Promise.all` or sequential awaits: Firstly we flatten the block tree, asynchronously convert all nodes, then rebuild the tree.
2. Insert the converted draft block tree in batches, **waiting for a tick after each _batch_ instead of after each _block_**.

This optimizes both block tree conversion and insertion while maintaining UI responsiveness.

## Performance Tests

### Comparing serial vs parallel (`Promise.all`) snapshot tree conversion:

- 1000 blocks: `Promise.all` 4.8ms, series 2.8ms
- 60 images: `Promise.all` 2.8ms, series 2.2ms

Serial conversion proved faster and was implemented in b90d736. The flatten-rebuild logic was retained for scalability.

### Pasting 1000 blocks performance:

Original: ~10s (see screen recording above)

New: ~1s with maintained UI responsiveness

https://github.com/user-attachments/assets/0202330a-66ce-47f3-b220-44a4e9c7c37d

### Switching to imported documents:

Loading the block tree into DOM (by switching to an imported document) causes similar delays in both implementations.

---

Legacy first take:

<details>

This PR significantly improves the performance of block import operations by addressing several inefficiencies in the current implementation:

- Removed the need to wait for `requestAnimationFrame` after each block insertion (see #7796), which was causing significant slowdowns.
- <del>Fixed the batch mechanism (see #7790) that wasn't properly enabled, replacing serial execution with true parallel processing.</del>
- Instead of executing all remaining tasks at once after 100 items (still causing UI freezes), we now process tasks in consistent batches of 100 per tick.
- Replaced `setTimeout` with `requestIdleCallback` for batch ticks, better utilizing idle browser time.

These optimizations result in much faster block imports while maintaining UI responsiveness. The new implementation processes 100 tasks per tick, striking a balance between speed and smooth user experience.

Tested using [clipboard-test.md](https://github.com/user-attachments/files/17088183/clipboard-test.md) with 1000 line and 600 blocks:

## Before (took ~10s to paste)

https://github.com/user-attachments/assets/3a6379c9-621e-490e-a79f-4377494d5915

## After (instant response without blocking UI)

https://github.com/user-attachments/assets/dea7c1f9-a8c1-4926-92b7-f5cb40957fad

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants