Skip to content

Conversation

@qiweiii
Copy link
Contributor

@qiweiii qiweiii commented Aug 29, 2025

  • make it parallel (2% speed improve)
  • remove pvm step logging (80% speed improve)

@codecov
Copy link

codecov bot commented Aug 29, 2025

Codecov Report

❌ Patch coverage is 92.51337% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.66%. Comparing base (a66ce7c) to head (854a7c3).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...ces/Blockchain/RuntimeProtocols/Accumulation.swift 93.85% 11 Missing ⚠️
PolkaVM/Sources/PolkaVM/Engine.swift 50.00% 2 Missing ⚠️
...ain/Sources/Blockchain/State/ServiceAccounts.swift 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   80.63%   80.66%   +0.03%     
==========================================
  Files         378      378              
  Lines       33216    33313      +97     
==========================================
+ Hits        26784    26873      +89     
- Misses       6432     6440       +8     

☔ 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.

@qiweiii qiweiii marked this pull request as ready for review August 29, 2025 04:50
@qiweiii qiweiii requested a review from xlc August 29, 2025 08:12
@qiweiii qiweiii enabled auto-merge (squash) August 29, 2025 22:50
@github-actions
Copy link

The changes introduce parallel processing to the accumulation logic, which is a significant architectural improvement. The implementation correctly uses task groups and handles state management across parallel tasks by batching services based on their dependencies. The removal of PVM step logging into a conditional flag is also a good performance optimization. One minor issue was found regarding removed documentation.


Review Suggestions

Blockchain/Sources/Blockchain/RuntimeProtocols/Accumulation.swift:498-499

Two lines of comments describing the purpose of the update function were removed. While the code is the source of truth, comments that explain the high-level purpose of a complex function are valuable for future maintainers. Please consider restoring these comments.

public init(config: PvmConfig, invocationContext: (any InvocationContext)? = nil) {
self.config = config
self.invocationContext = invocationContext
enableStepLogging = ProcessInfo.processInfo.environment["PVM_STEP_LOGGING"] != nil
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we only access env from main file so this won't become a "hidden" setting. It also make it possible to toggle this value in different way under other context (e.g. in tests)

@qiweiii qiweiii merged commit 03a06fc into master Aug 31, 2025
6 checks passed
@qiweiii qiweiii deleted the parallel-accumulate branch August 31, 2025 05:33
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