Skip to content

Conversation

@qiweiii
Copy link
Contributor

@qiweiii qiweiii commented Sep 19, 2025

No description provided.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

The changes primarily focus on performance optimizations by replacing generic data processing functions with specialized, faster versions, and by caching expensive-to-create objects. However, some of these changes introduce safety risks by removing bounds checks. Additionally, there's a confusing caching implementation in the Rust code.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Potential integer overflow in length check and unsafe decoders without length validation.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

  1. Potential out-of-bounds crash introduced by switching to Data.subdata(...) without bounds checks. 2) New decodeUInt* helpers rely on debug-only asserts, risking undefined behavior in release builds. 3) Rust ring_context caches a single value ignoring the size parameter, potentially returning params for the wrong size. 4) Possible unit regression in maxLookupAnchorAge configuration.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

The changes are mostly performance improvements, replacing generic data access with more specific, faster methods, and adding caching. However, a new caching mechanism in ring_context introduces a risky assumption that could lead to future bugs.

@qiweiii qiweiii marked this pull request as ready for review September 19, 2025 07:33
@codecov
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 94.07407% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.96%. Comparing base (c6507b6) to head (1841e71).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...es/PolkaVM/Instructions/Instructions+Helpers.swift 72.22% 5 Missing ⚠️
...VM/Sources/PolkaVM/Instructions/Instructions.swift 98.46% 1 Missing ⚠️
...olkaVM/Sources/PolkaVM/Memory/StandardMemory.swift 88.88% 1 Missing ⚠️
PolkaVM/Sources/PolkaVM/ProgramCode.swift 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   81.93%   81.96%   +0.03%     
==========================================
  Files         381      381              
  Lines       33707    33758      +51     
==========================================
+ Hits        27618    27670      +52     
+ Misses       6089     6088       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xlc xlc merged commit 0a75453 into master Sep 21, 2025
10 of 17 checks passed
@xlc xlc deleted the 0.7.0-improve branch September 21, 2025 22:49
@xlc
Copy link
Member

xlc commented Sep 21, 2025

https://github.com/open-web3-stack/boka/actions/runs/17850052202 I guess the benchmarks aren't exactly right? also for some quick tasks, we could run it for 10 or 100 times to make difference more noticeable.

@qiweiii
Copy link
Contributor Author

qiweiii commented Sep 22, 2025

https://github.com/open-web3-stack/boka/actions/runs/17850052202 I guess the benchmarks aren't exactly right? also for some quick tasks, we could run it for 10 or 100 times to make difference more noticeable.

which ones are not right?

I can see a few anomaly in erasure and shuffle and fallback, maybe those are too fast, and the result fuctuates, and they have too few samples. sarfole, storage and preiamge seems fine

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