Additional peformance improvements and metrics for parallel execution#3197
Additional peformance improvements and metrics for parallel execution#3197MitchTurner wants to merge 18 commits intocreate_exec_sequencer_skeletonfrom
Conversation
|
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Codex <c***@.local>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Fuel Labs Contributor License Agreement and this Pull Request will be revalidated. |
PR SummaryMedium Risk Overview Introduces new Prometheus metrics for parallel executor and adds timing logs. A new Updates parallel executor + txpool integration for better selection. Parallel executor config renames Changes txpool v2 selection algorithm and config. Adds Reviewed by Cursor Bugbot for commit 3ee3d1c. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| max_pending_read_pool_requests: 1000, | ||
| }, | ||
| pending_pool_tx_ttl: Duration::from_secs(3), | ||
| pending_pool_tx_ttl: Duration::from_secs(100), |
There was a problem hiding this comment.
Default pending pool TTL changed from 3s to 100s
Medium Severity
The pending_pool_tx_ttl default was changed from Duration::from_secs(3) to Duration::from_secs(100), a 33x increase. This default is picked up by Config::local_node_with_configs via ..Default::default(), affecting local node configurations and all tests that don't explicitly override it. This appears to be an accidentally committed benchmark tuning value rather than an intentional production change, especially since the PR is focused on tracing/metrics and doesn't mention this behavioral change.
| "process_block elapsed time: {:?} for block: {:?}", | ||
| instant.elapsed(), | ||
| height | ||
| ); |
There was a problem hiding this comment.
Debug warn-level logging in block processing hot path
High Severity
Seven tracing::warn! timing statements were added to process_block in worker_service.rs, and two warn! statements were added to the importer's run loop. These fire on every block processed, flooding production logs with routine timing data at warn level. The warn level is typically reserved for unusual conditions, and these are clearly benchmark instrumentation that escaped into production code outside the parallel executor.
Additional Locations (1)
| select_time_us = select_elapsed.as_micros(), | ||
| "txpool_v2 selection summary" | ||
| ); | ||
|
|
There was a problem hiding this comment.
Debug warn logging in txpool selection and worker
Medium Severity
Multiple tracing::warn! calls were added to extract_transactions_for_block in pool.rs, to insert error/missing-input handling in pool_worker.rs, to extract_block_transactions, and to gather_best_txs in ratio_tip_gas.rs. These fire on every transaction insertion and every block extraction, producing excessive warn-level logs in production. These are benchmark diagnostics outside the parallel executor scope.
Additional Locations (2)
| parallel_executor_metrics::record_execution_time(execution_time); | ||
| execution_time_recorded = true; | ||
| } | ||
| tracing::warn!("******"); |
There was a problem hiding this comment.
Debug asterisk markers left in scheduler code
Low Severity
Two tracing::warn!("******") calls remain in the scheduler at key control-flow points (deadline reached and before waiting for execution tasks). These are clearly personal debugging markers that were not cleaned up before committing.
Additional Locations (1)
| }; | ||
|
|
||
| const PATH_SNAPSHOT: &str = "/Users/green/fuel/fuel-core-2/benches/local-testnet"; | ||
| const PATH_SNAPSHOT: &str = "./local-testnet"; |
There was a problem hiding this comment.
Benchmark snapshot path changed to incorrect relative path
High Severity
The PATH_SNAPSHOT constant was changed from an absolute path to "./local-testnet", which is a gitignored directory. The benchmark will fail when run from most working directories since it expects the snapshot in the current directory. The correct path should be "../bin/fuel-core/chainspec/local-testnet" as used in other parts of the codebase, or the absolute path should be removed in favor of a configurable parameter rather than hardcoded.
| }) | ||
| .sum(); | ||
| test_builder.gas_limit = Some(gas_limit * 4); | ||
| test_builder.gas_limit = Some(gas_limit); |
There was a problem hiding this comment.
Benchmark gas limit reduced by 75 percent
Medium Severity
The block gas limit calculation was changed from gas_limit * 4 to just gas_limit, reducing it by 75%. This sets the gas limit to exactly the sum of transaction gas requirements without any buffer. This could cause the benchmark to fail if there's any gas overhead or if transactions use more gas than their maximum estimate, and significantly changes benchmark behavior in a way unrelated to the PR's stated purpose of adding tracing and metrics.
|
|
||
| pub fn set_total_gas_used(gas: u64) { | ||
| parallel_executor_metrics().total_gas_used.set(gas as i64); | ||
| } |
There was a problem hiding this comment.
Gas metric cast can overflow producing negative values
Low Severity
The total_gas_used metric accepts a u64 value but stores it in an i64 Gauge without validation. The gas accumulation in the scheduler uses saturating_add, allowing values up to u64::MAX. When cast to i64, values exceeding i64::MAX wrap to negative numbers, producing nonsensical negative gas metrics that could mislead operators.
Additional Locations (1)
| forbid_fake_coins, | ||
| at_height, | ||
| record_storage_read_replay, | ||
| ), |
There was a problem hiding this comment.
Duplicate forbid_fake_coins passed instead of two distinct args
High Severity
In the newly added ParallelExecutorAdapterInner::Native branch of dry_run, forbid_fake_coins is passed for both the second and third positional arguments of native.executor.dry_run(...). The underlying executor's dry_run likely takes separate forbid_fake_coins and forbid_fake_utxo parameters (5 args total vs the trait's 4), so passing forbid_fake_coins twice means the wrong value is used for one of those parameters. This was copied from the existing ExecutorAdapter implementation above, but since this is new code path for the ParallelExecutorAdapter, it propagates the bug to a new call site.
| ) | ||
| }) | ||
| .unwrap_or(false) | ||
| } |
There was a problem hiding this comment.
Environment variable checked on every tx selection call
Medium Severity
contract_anchor_bias_enabled() reads std::env::var("TXPOOL_V2_CONTRACT_ANCHOR_BIAS") on every call to gather_best_txs, which is invoked for every block's transaction selection. Using environment variables as a runtime toggle for core selection algorithm behavior is fragile, untestable, and introduces syscall overhead on a hot path. This looks like a temporary development toggle that wasn't replaced with a proper config field.
MitchTurner
changed the title
| ))?; | ||
| let mut total_gas: u64 = 0; | ||
|
|
||
| tracing::warn!("scheduler starting run loop at {:?}", instant.elapsed()); |
There was a problem hiding this comment.
Warn-level debug logs in scheduler and executor
Medium Severity
Numerous tracing::warn! timing/debug statements were added in the parallel executor scheduler, executor, and txpool pool worker. Messages like "******", "scheduler starting run loop", "Waiting for workers to finish", and "txpool_v2 insert" emit on every block production cycle at warn level. These are benchmark instrumentation that will spam production logs.
Additional Locations (2)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 11 total unresolved issues (including 10 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Commented-out code blocks left in scheduler
- Removed the stale commented-out
verify_coherency_and_merge_resultsand tracing debug blocks from scheduler, pool worker, and ratio-tip-gas selection code.
- Removed the stale commented-out
Or push these changes by commenting:
@cursor push 8637c345cd
Preview (8637c345cd)
diff --git a/crates/services/parallel-executor/src/scheduler.rs b/crates/services/parallel-executor/src/scheduler.rs
--- a/crates/services/parallel-executor/src/scheduler.rs
+++ b/crates/services/parallel-executor/src/scheduler.rs
@@ -682,12 +682,6 @@
}
}
- // let mut res = self.verify_coherency_and_merge_results(
- // nb_batch_created,
- // l1_execution_data,
- // storage_with_da.clone(),
- // )?;
-
let result = self.verify_coherency_and_merge_results(
nb_batch_created,
l1_execution_data,
diff --git a/crates/services/txpool_v2/src/pool_worker.rs b/crates/services/txpool_v2/src/pool_worker.rs
--- a/crates/services/txpool_v2/src/pool_worker.rs
+++ b/crates/services/txpool_v2/src/pool_worker.rs
@@ -425,12 +425,6 @@
}
let resolved_txs =
self.pending_pool.new_known_tx(tx.utxo_ids_with_outputs());
- // tracing::warn!(
- // result = "ok",
- // pending_pool_txs = self.pending_pool.current_txs,
- // resolved_txs = resolved_txs.len(),
- // "txpool_v2 insert"
- // );
for (tx, source) in resolved_txs {
if let Err(e) = self
diff --git a/crates/services/txpool_v2/src/selection_algorithms/ratio_tip_gas.rs b/crates/services/txpool_v2/src/selection_algorithms/ratio_tip_gas.rs
--- a/crates/services/txpool_v2/src/selection_algorithms/ratio_tip_gas.rs
+++ b/crates/services/txpool_v2/src/selection_algorithms/ratio_tip_gas.rs
@@ -798,12 +798,6 @@
storage_id,
&Self::collect_contract_inputs(store_entry),
);
- // tracing::warn!(
- // executable_set_size = self
- // .executable_transactions_sorted_tip_gas_ratio
- // .len(),
- // "txpool_v2 executable added"
- // );
}
fn get_less_worth_txs(&self) -> impl Iterator<Item = &Self::StorageIndex> {
@@ -822,12 +816,6 @@
key,
&Self::collect_contract_inputs(storage_entry),
);
- // tracing::warn!(
- // executable_set_size = self
- // .executable_transactions_sorted_tip_gas_ratio
- // .len(),
- // "txpool_v2 executable removed"
- // );
}
fn number_of_executable_transactions(&self) -> usize {This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3ee3d1c. Configure here.
| // nb_batch_created, | ||
| // l1_execution_data, | ||
| // storage_with_da.clone(), | ||
| // )?; |
There was a problem hiding this comment.
Commented-out code blocks left in scheduler
Low Severity
Several blocks of commented-out code were left in the scheduler (the original verify_coherency_and_merge_results call) and the pool worker (tracing for insert results). These appear to be debugging artifacts that were not cleaned up before the PR.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3ee3d1c. Configure here.
1 participant
Linked Issues/PRs
Description
Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]