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 DomSlot debugging #3817

Merged
merged 2 commits into from
Mar 4, 2025
Merged

Conversation

WorldSEnder
Copy link
Member

@WorldSEnder WorldSEnder commented Mar 3, 2025

Description

Formatting the internal DomSlot struct for debugging could cause a panic if this was done before hydration of an element finished (There are multiple ways to invoke this from user code, such as formatting a Handle).

This was only a problem in code with debug_assertions enabled.

Checklist

  • I have reviewed my own code
  • I have added tests

github-actions[bot]
github-actions bot previously approved these changes Mar 3, 2025
Copy link

github-actions bot commented Mar 3, 2025

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)

✅ None of the examples has changed their size significantly.

Copy link

github-actions bot commented Mar 3, 2025

Visit the preview URL for this PR (updated for commit f657df0):

https://yew-rs-api--pr3817-domslot-debug-p3q90mow.web.app

(expires Mon, 10 Mar 2025 16:03:05 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link

github-actions bot commented Mar 3, 2025

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.745 ns      │ 4.774 ns      │ 2.751 ns      │ 2.824 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.771 ns      │ 3.477 ns      │ 2.776 ns      │ 2.79 ns       │ 100     │ 1000000000

Copy link

github-actions bot commented Mar 3, 2025

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 310.514 311.752 310.788 0.382
Hello World 10 477.468 534.084 486.918 17.078
Function Router 10 1608.430 1621.247 1615.428 4.173
Concurrent Task 10 1006.008 1006.668 1006.373 0.174
Many Providers 10 1118.359 1150.187 1134.245 10.544

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 310.524 313.565 311.169 0.885
Hello World 10 470.732 486.764 475.771 5.483
Function Router 10 1605.956 1625.623 1612.332 5.457
Concurrent Task 10 1005.756 1006.698 1006.357 0.269
Many Providers 10 1137.118 1161.844 1145.394 7.027

@Madoshakalaka
Copy link
Member

I'm not sure what the motivation behind the TRAP is. It seems to be used as next_sibling placeholders? Why can't we make next sibling optional?

Copy link
Member

@Madoshakalaka Madoshakalaka left a comment

Choose a reason for hiding this comment

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

Tried this in a large SSR project that previously panicked over the trap, with and without release mode. Both work fine!

@WorldSEnder
Copy link
Member Author

I'm not sure what the motivation behind the TRAP is. It seems to be used as next_sibling placeholders? Why can't we make next sibling optional?

It is a placeholder. Note that it's only used when debug_assertions are enabled. It works in a non-intrusive way without changing the representation of next_sibling. A next_sibling of None means "at the end of the children list", while this represents the fact that we don't know yet. Debug assertions check that if we don't know yet we don't make use of the next sibling to move stuff around and should fall into the trap.

@WorldSEnder WorldSEnder merged commit a3a3ffc into yewstack:master Mar 4, 2025
25 checks passed
@WorldSEnder WorldSEnder deleted the domslot-debug branch March 4, 2025 08:36
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.

2 participants