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(core): fix reference preview flickering and improve loading #7563

Merged
merged 10 commits into from
Oct 8, 2024

Conversation

bjoerge
Copy link
Member

@bjoerge bjoerge commented Sep 30, 2024

Description

This PR fixes various loading and rendering issues with reference previews. It should significantly reduce the number of re-renderings and layout shifts happening as we load what's needed to preview references in the Studio. These issues are particularily visible behind slower networks.

Here's before/after with network throttling set to 3g
Before
https://github.com/user-attachments/assets/1b87897e-0abf-49db-8a2e-edb3669e2353

After
https://github.com/user-attachments/assets/a37704a0-4b52-4f94-8f76-3a2101cc0f2e

Included some additional improvments here as well:

What to review

  • Modifications to the ReferenceInput and related components
  • Improvements in the useReferenceInfo hook and getReferenceInfo function
  • Updates to the PreviewLoader and useValuePreview hook
  • Changes to the useVisibility hook and its implementation

Testing

Should be covered by existing tests

Notes for release

  • Improved loading state of reference previews

Copy link

vercel bot commented Sep 30, 2024

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

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 7, 2024 4:01pm
performance-studio ✅ Ready (Inspect) Visit Preview Oct 7, 2024 4:01pm
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 7, 2024 4:01pm
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 7, 2024 4:01pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 7, 2024 4:01pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 4:01pm

Copy link
Member Author

bjoerge commented Sep 30, 2024

@bjoerge bjoerge changed the title fix(core) use rxSwr for availability fix(core): fix reference preview flickering and improve loading Sep 30, 2024
Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Oct 7, 2024

⚡️ Editor Performance Report

Updated Mon, 07 Oct 2024 16:08:20 GMT

Benchmark reference
latency of sanity@latest
experiment
latency of this branch
Δ (%)
latency difference
article (title) 18.0 efps (56ms) 17.2 efps (58ms) +3ms (+4.5%)
article (body) 56.2 efps (18ms) 58.0 efps (17ms) -1ms (-3.1%)
article (string inside object) 19.6 efps (51ms) 18.2 efps (55ms) +4ms (+7.8%)
article (string inside array) 14.8 efps (68ms) 14.5 efps (69ms) +2ms (+2.2%)
recipe (name) 29.9 efps (34ms) 30.3 efps (33ms) -1ms (-1.5%)
recipe (description) 34.5 efps (29ms) 33.3 efps (30ms) +1ms (+3.4%)
recipe (instructions) 99.9+ efps (7ms) 99.9+ efps (7ms) +0ms (-/-%)
synthetic (title) 14.7 efps (68ms) 15.6 efps (64ms) -4ms (-5.9%)
synthetic (string inside object) 16.1 efps (62ms) 15.3 efps (66ms) +4ms (+5.6%)

efps — editor "frames per second". The number of updates assumed to be possible within a second.

Derived from input latency. efps = 1000 / input_latency

Detailed information

🏠 Reference result

The performance result of sanity@latest

Benchmark latency p75 p90 p99 blocking time test duration
article (title) 56ms 65ms 119ms 269ms 1346ms 14.4s
article (body) 18ms 22ms 56ms 180ms 360ms 6.2s
article (string inside object) 51ms 57ms 64ms 137ms 844ms 8.8s
article (string inside array) 68ms 72ms 82ms 209ms 1731ms 9.8s
recipe (name) 34ms 36ms 55ms 72ms 120ms 9.7s
recipe (description) 29ms 31ms 36ms 58ms 42ms 6.4s
recipe (instructions) 7ms 9ms 10ms 11ms 0ms 3.3s
synthetic (title) 68ms 70ms 81ms 297ms 2178ms 17.5s
synthetic (string inside object) 62ms 66ms 70ms 294ms 1727ms 9.6s

🧪 Experiment result

The performance result of this branch

Benchmark latency p75 p90 p99 blocking time test duration
article (title) 58ms 63ms 71ms 195ms 1298ms 13.6s
article (body) 17ms 20ms 26ms 189ms 288ms 5.9s
article (string inside object) 55ms 58ms 64ms 168ms 983ms 8.8s
article (string inside array) 69ms 73ms 115ms 198ms 1885ms 10.3s
recipe (name) 33ms 35ms 41ms 85ms 36ms 9.3s
recipe (description) 30ms 32ms 35ms 86ms 69ms 6.4s
recipe (instructions) 7ms 9ms 10ms 11ms 0ms 3.3s
synthetic (title) 64ms 67ms 83ms 369ms 2082ms 16.1s
synthetic (string inside object) 66ms 71ms 85ms 446ms 2187ms 10.2s

📚 Glossary

column definitions

  • benchmark — the name of the test, e.g. "article", followed by the label of the field being measured, e.g. "(title)".
  • latency — the time between when a key was pressed and when it was rendered. derived from a set of samples. the median (p50) is shown to show the most common latency.
  • p75 — the 75th percentile of the input latency in the test run. 75% of the sampled inputs in this benchmark were processed faster than this value. this provides insight into the upper range of typical performance.
  • p90 — the 90th percentile of the input latency in the test run. 90% of the sampled inputs were faster than this. this metric helps identify slower interactions that occurred less frequently during the benchmark.
  • p99 — the 99th percentile of the input latency in the test run. only 1% of sampled inputs were slower than this. this represents the worst-case scenarios encountered during the benchmark, useful for identifying potential performance outliers.
  • blocking time — the total time during which the main thread was blocked, preventing user input and UI updates. this metric helps identify performance bottlenecks that may cause the interface to feel unresponsive.
  • test duration — how long the test run took to complete.

@bjoerge
Copy link
Member Author

bjoerge commented Oct 7, 2024

Needs a new review after rebasing. The failing e2e tests are not related to the changes here and will be fixed by #7601

Copy link
Contributor

@pedrobonamin pedrobonamin left a comment

Choose a reason for hiding this comment

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

Thanks for this, been testing it and it makes a great difference!

Just a bit worried about the removal of the key in the document panel. There are not too many ways to test that, but it seems the panel is unmounted anyways when we are navigating to a new document.

Do you have any thoughts on that?

@bjoerge bjoerge added this pull request to the merge queue Oct 8, 2024
Merged via the queue into next with commit 1e31c35 Oct 8, 2024
64 checks passed
@bjoerge bjoerge deleted the fix-reference-previews branch October 8, 2024 10:08
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.

3 participants