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

Add Sync impl to ResumableInvocation #870

Merged
merged 2 commits into from
Dec 26, 2023
Merged

Conversation

Robbepop
Copy link
Member

Closes #869.

This implicitly also grants Sync to the TypedResumableInvocation type.

@tomaka

@tomaka
Copy link
Contributor

tomaka commented Dec 26, 2023

I'm personally fine with that, but as I mentioned in the issue are you sure that it's not for example Stack that is missing the trait implementation instead?

@paritytech-cicd-pr
Copy link

BENCHMARKS

NATIVEWASMTIME
BENCHMARKMASTERPRDIFFMASTERPRDIFFWASMTIME OVERHEAD
execute/
br_table
1.48ms 1.51ms 🔴 2.00% 1.36ms 1.35ms ⚪ -1.21% 🟢 -11%
execute/
call/host/1
50.74µs 50.76µs ⚪ 0.03% 64.09µs 64.56µs ⚪ 0.72% 🟢 27%
execute/
call/rec
170.32µs 171.68µs ⚪ 0.80% 329.19µs 329.77µs ⚪ 0.18% 🟡 92%
execute/
count_until
5.33ms 5.34ms ⚪ 0.05% 7.48ms 7.50ms ⚪ 0.19% 🟢 40%
execute/
divrem
6.34ms 6.33ms ⚪ -0.20% 6.99ms 6.98ms ⚪ -0.12% 🟢 10%
execute/
factorial/iter
269.48µs 267.40µs ⚪ -0.77% 344.57µs 332.58µs 🟢 -3.48% 🟢 24%
execute/
factorial/rec
716.85µs 711.91µs ⚪ -0.69% 1.23ms 1.22ms ⚪ -0.67% 🟡 72%
execute/
fibonacci/iter
1.33ms 1.33ms ⚪ 0.03% 1.23ms 1.24ms ⚪ 0.13% 🟢 -7%
execute/
fibonacci/rec
6.19ms 6.24ms ⚪ 0.76% 12.23ms 12.50ms 🔴 2.17% 🟡 100%
execute/
fibonacci/tail
1.27ms 1.27ms ⚪ 0.11% 4.39ms 4.41ms ⚪ 0.48% 🔴 246%
execute/
fuse
7.07ms 7.07ms ⚪ 0.00% 11.18ms 11.17ms ⚪ -0.06% 🟡 58%
execute/
global/bump
1.32ms 1.32ms ⚪ -0.20% 1.59ms 1.59ms ⚪ 0.10% 🟢 21%
execute/
global/get_const
482.20µs 482.14µs ⚪ -0.01% 748.35µs 747.80µs ⚪ -0.07% 🟡 55%
execute/
is_even/rec
1.13ms 1.15ms 🔴 2.03% 2.09ms 2.08ms ⚪ -0.09% 🟡 81%
execute/
memory/fill_bytes
1.12ms 1.12ms ⚪ 0.15% 1.23ms 1.24ms ⚪ 0.14% 🟢 11%
execute/
memory/sum_bytes
1.09ms 1.09ms ⚪ -0.11% 1.39ms 1.39ms ⚪ 0.09% 🟢 28%
execute/
memory/vec_add
2.94ms 2.94ms ⚪ -0.03% 3.44ms 3.45ms ⚪ 0.31% 🟢 17%
execute/
recursive_scan
192.58µs 193.33µs ⚪ 0.39% 353.16µs 354.46µs ⚪ 0.37% 🟡 83%
execute/
recursive_trap
16.51µs 15.92µs 🟢 -3.54% 32.21µs 32.01µs ⚪ -0.62% 🔴 101%
execute/
regex_redux
586.38µs 586.45µs ⚪ 0.01% 1.12ms 1.14ms 🔴 2.19% 🟡 95%
execute/
rev_complement
439.57µs 439.53µs ⚪ 0.00% 672.58µs 673.28µs ⚪ 0.10% 🟡 53%
execute/
tiny_keccak
351.93µs 352.14µs ⚪ 0.06% 380.28µs 379.61µs ⚪ -0.18% 🟢 8%
execute/
trunc_f2i
612.72µs 613.05µs ⚪ 0.05% 948.98µs 946.72µs ⚪ -0.24% 🟡 54%
instantiate/
wasm_kernel
54.27µs 56.31µs 🔴 3.76% 56.60µs 55.34µs ⚪ -2.23% 🟢 -2%
overhead/
call/typed/0
1.27ms 1.28ms ⚪ 0.58% 888.52µs 865.39µs 🟢 -2.60% 🟢 -32%
overhead/
call/typed/16
1.64ms 1.65ms ⚪ 0.18% 2.50ms 2.55ms 🔴 1.90% 🟡 55%
overhead/
call/untyped/0
1.58ms 1.59ms ⚪ 0.76% 1.15ms 1.18ms 🔴 2.28% 🟢 -26%
overhead/
call/untyped/16
2.43ms 2.46ms ⚪ 0.97% 4.28ms 4.30ms ⚪ 0.34% 🟡 75%
translate/
bz2/checked/eager/default
1.33ms 1.32ms ⚪ -0.39% 2.44ms 2.49ms 🔴 2.09% 🟡 89%
translate/
bz2/checked/eager/fuel
1.44ms 1.42ms ⚪ -0.76% 2.65ms 2.68ms ⚪ 1.22% 🟡 88%
translate/
bz2/checked/lazy-translation/default
540.75µs 541.66µs ⚪ 0.17% 977.65µs 1.10ms 🔴 12.67% 🔴 103%
translate/
bz2/checked/lazy/default
37.21µs 37.04µs ⚪ -0.44% 45.66µs 46.02µs ⚪ 0.80% 🟢 24%
translate/
bz2/unchecked/eager/default
1.08ms 1.09ms ⚪ 0.18% 1.86ms 1.87ms ⚪ 0.54% 🟡 73%
translate/
erc1155/checked/eager/default
281.06µs 277.44µs 🟢 -1.29% 480.40µs 482.58µs ⚪ 0.45% 🟡 74%
translate/
erc1155/checked/eager/fuel
303.06µs 297.92µs 🟢 -1.69% 514.79µs 515.77µs ⚪ 0.19% 🟡 73%
translate/
erc1155/checked/lazy-translation/default
127.21µs 126.21µs ⚪ -0.78% 213.13µs 235.82µs 🔴 10.65% 🟡 87%
translate/
erc1155/checked/lazy/default
26.31µs 25.83µs 🟢 -1.83% 32.55µs 32.51µs ⚪ -0.12% 🟢 26%
translate/
erc1155/unchecked/eager/default
232.21µs 229.93µs ⚪ -0.98% 371.98µs 371.93µs ⚪ -0.02% 🟡 62%
translate/
erc20/checked/eager/default
136.96µs 134.92µs 🟢 -1.49% 229.77µs 232.13µs ⚪ 1.03% 🟡 72%
translate/
erc20/checked/eager/fuel
145.69µs 143.51µs 🟢 -1.50% 242.18µs 245.06µs ⚪ 1.19% 🟡 71%
translate/
erc20/checked/lazy-translation/default
65.31µs 65.40µs ⚪ 0.13% 107.06µs 116.99µs 🔴 9.28% 🟡 79%
translate/
erc20/checked/lazy/default
19.18µs 19.24µs ⚪ 0.35% 24.57µs 24.75µs ⚪ 0.74% 🟢 29%
translate/
erc20/unchecked/eager/default
112.03µs 111.24µs ⚪ -0.71% 177.46µs 177.77µs ⚪ 0.18% 🟡 60%
translate/
erc721/checked/eager/default
194.01µs 191.20µs 🟢 -1.45% 333.16µs 336.52µs ⚪ 1.01% 🟡 76%
translate/
erc721/checked/eager/fuel
205.39µs 201.99µs 🟢 -1.66% 351.47µs 351.75µs ⚪ 0.08% 🟡 74%
translate/
erc721/checked/lazy-translation/default
90.93µs 90.34µs ⚪ -0.65% 154.83µs 166.48µs 🔴 7.52% 🟡 84%
translate/
erc721/checked/lazy/default
23.13µs 22.40µs 🟢 -3.14% 28.85µs 28.72µs ⚪ -0.45% 🟢 28%
translate/
erc721/unchecked/eager/default
157.79µs 156.06µs ⚪ -1.10% 252.42µs 251.96µs ⚪ -0.19% 🟡 61%
translate/
pulldown_cmark/checked/eager/default
3.61ms 3.58ms ⚪ -0.97% 6.25ms 6.32ms ⚪ 1.15% 🟡 77%
translate/
pulldown_cmark/checked/eager/fuel
3.91ms 3.86ms 🟢 -1.13% 6.76ms 6.79ms ⚪ 0.44% 🟡 76%
translate/
pulldown_cmark/checked/lazy-translation/default
1.53ms 1.51ms 🟢 -1.44% 2.58ms 2.88ms 🔴 11.45% 🟡 91%
translate/
pulldown_cmark/checked/lazy/default
254.64µs 228.93µs 🟢 -10.10% 256.09µs 244.18µs 🟢 -4.65% 🟢 7%
translate/
pulldown_cmark/unchecked/eager/default
3.04ms 3.01ms ⚪ -1.01% 4.88ms 4.85ms ⚪ -0.62% 🟡 61%
translate/
spidermonkey/checked/eager/default
76.68ms 76.29ms ⚪ -0.51% 137.07ms 136.96ms ⚪ -0.08% 🟡 80%
translate/
spidermonkey/checked/eager/fuel
82.99ms 82.63ms ⚪ -0.43% 147.18ms 147.68ms ⚪ 0.33% 🟡 79%
translate/
spidermonkey/checked/lazy-translation/default
32.36ms 32.28ms ⚪ -0.24% 56.30ms 62.48ms 🔴 10.97% 🟡 94%
translate/
spidermonkey/checked/lazy/default
3.28ms 3.24ms ⚪ -1.09% 3.85ms 3.87ms ⚪ 0.57% 🟢 19%
translate/
spidermonkey/unchecked/eager/default
64.21ms 63.80ms ⚪ -0.63% 104.53ms 104.73ms ⚪ 0.19% 🟡 64%
translate/
wasm_kernel/checked/eager/default
5.09ms 5.06ms ⚪ -0.50% 8.80ms 8.90ms ⚪ 1.10% 🟡 76%
translate/
wasm_kernel/checked/eager/fuel
5.25ms 5.22ms ⚪ -0.56% 9.24ms 9.31ms ⚪ 0.75% 🟡 78%
translate/
wasm_kernel/checked/lazy-translation/default
2.39ms 2.39ms ⚪ -0.21% 3.99ms 4.41ms 🔴 10.49% 🟡 85%
translate/
wasm_kernel/checked/lazy/default
425.99µs 420.01µs ⚪ -1.40% 478.58µs 484.80µs ⚪ 1.30% 🟢 15%
translate/
wasm_kernel/unchecked/eager/default
4.14ms 4.13ms ⚪ -0.25% 6.72ms 6.70ms ⚪ -0.19% 🟡 62%

Link to pipeline

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (688d986) 80.81% compared to head (99e4e34) 80.81%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #870   +/-   ##
=======================================
  Coverage   80.81%   80.81%           
=======================================
  Files         256      256           
  Lines       22908    22908           
=======================================
  Hits        18513    18513           
  Misses       4395     4395           

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

@Robbepop
Copy link
Member Author

I'm personally fine with that, but as I mentioned in the issue are you sure that it's not for example Stack that is missing the trait implementation instead?

Stack is not a public type, so won't make a difference to the user. Also unlike ResumableInvocation, Stack does not own an Engine and thus it is not guaranteed that it is actually safe to do anything with it since it could have outlived the Engine and thus contain invalid pointers by itself.

@Robbepop Robbepop merged commit bd5b613 into master Dec 26, 2023
20 checks passed
@Robbepop Robbepop deleted the rf-sync-resumable-invocation branch December 26, 2023 09: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.

ResumableInvocation doesn't implement Sync
4 participants