Ensure that uncommitted transactions are always removed from QoS #32285

buffalu · 2023-06-26T21:29:57Z

Problem

When feature apply_cost_tracker_during_replay is applied, transactions that are added to QoS but aren't executed or committed will never get removed. This will result in an overestimation of actual compute units used, resulting in smaller blocks.

Summary of Changes

Uncommitted or unrecorded transaction costs always remove costs. Costs are only updated if apply_cost_tracker_during_replay is applied

apfitzge

Implementation itself looks good to me. Some nit-picking (as always!) on a comment.

core/src/banking_stage/consumer.rs

apfitzge

I'm happy with these changes as is, but requested a review from @taozhu-chicago as well. Since this is primarily related to his feature-gate for not updating costs, I will defer final approval to him.

tao-stones

Change looks good to me. Just few nit & comments

core/src/banking_stage/consumer.rs

core/src/banking_stage/qos_service.rs

buffalu · 2023-06-27T22:54:19Z

@taozhu-chicago some benchmarks below on consumer. overall seems the same

server: c2-standard-30 sitting idle
command: ./cargo nightly bench -p solana-core --bench consumer

commit a9239f1

test bench_process_and_record_transactions_full_batch                        ... bench:     997,244 ns/iter (+/- 22,707)
test bench_process_and_record_transactions_full_batch_disable_tx_cost_update ... bench:   1,021,068 ns/iter (+/- 38,027)
test bench_process_and_record_transactions_half_batch                        ... bench:     484,428 ns/iter (+/- 24,964)
test bench_process_and_record_transactions_half_batch_disable_tx_cost_update ... bench:     485,397 ns/iter (+/- 15,520)
test bench_process_and_record_transactions_unbatched                         ... bench:      34,607 ns/iter (+/- 1,616)
test bench_process_and_record_transactions_unbatched_disable_tx_cost_update  ... bench:      34,742 ns/iter (+/- 1,796)

commit 1b2b825 (commit this PR started on with modified core/benches/consumer.rs matching a92..)

test bench_process_and_record_transactions_full_batch                        ... bench:   1,000,160 ns/iter (+/- 26,487)
test bench_process_and_record_transactions_full_batch_disable_tx_cost_update ... bench:   1,002,358 ns/iter (+/- 43,635)
test bench_process_and_record_transactions_half_batch                        ... bench:     484,916 ns/iter (+/- 22,816)
test bench_process_and_record_transactions_half_batch_disable_tx_cost_update ... bench:     483,655 ns/iter (+/- 9,251)
test bench_process_and_record_transactions_unbatched                         ... bench:      35,777 ns/iter (+/- 769)
test bench_process_and_record_transactions_unbatched_disable_tx_cost_update  ... bench:      35,607 ns/iter (+/- 612)

tao-stones

lgtm - thanks for benching too

buffalu · 2023-06-28T01:22:56Z

@taozhu-chicago or @t-nelson can you add CI label to kick off CI

core/benches/consumer.rs

apfitzge · 2023-06-28T15:42:01Z

core/benches/consumer.rs

@@ -155,15 +166,30 @@ fn bench_process_and_record_transactions(bencher: &mut Bencher, batch_size: usiz

 #[bench]


this makes me wish there was a test_case-like crate for benches 😢
I guess these will only live until the feature is activated everywhere

apfitzge · 2023-06-28T16:30:33Z

Looks like we got a broken test. I think bank_process_and_record_transactions_cost_tracker needs to be updated to calculate the expected block cost with the uncommitted tx's cost removed.

buffalu · 2023-06-28T17:58:45Z

Looks like we got a broken test. I think bank_process_and_record_transactions_cost_tracker needs to be updated to calculate the expected block cost with the uncommitted tx's cost removed.

working on it now, will push up soon :)

core/src/banking_stage/consumer.rs

buffalu · 2023-06-28T18:46:54Z

@t-nelson hi i'd like a wendy's daves triple combo, a small chocolate frosty, and permission to trigger CI on my builds going into the solana labs repos :)

tao-stones

tiny nit

core/src/banking_stage/qos_service.rs

Co-authored-by: Tao Zhu <[email protected]>

codecov · 2023-06-28T19:35:18Z

Codecov Report

Merging #32285 (cc1c8b4) into master (9fb105c) will decrease coverage by 0.1%.
The diff coverage is 98.3%.

@@            Coverage Diff            @@
##           master   #32285     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         772      772             
  Lines      209538   209570     +32     
=========================================
+ Hits       171973   171984     +11     
- Misses      37565    37586     +21

buffalu · 2023-06-28T19:40:10Z

plz kick CI one more time, last build passed :)

apfitzge

lgtm

buffalu · 2023-06-28T20:31:42Z

🎊

buffalu · 2023-06-28T20:41:52Z

build passing, plz merge if happy

) Co-authored-by: Tao Zhu <[email protected]> (cherry picked from commit 5dee2e4)

…oS (backport of #32285) (#32320) Ensure that uncommitted transactions are always removed from QoS (#32285) Co-authored-by: Tao Zhu <[email protected]> (cherry picked from commit 5dee2e4) Co-authored-by: buffalu <[email protected]>

buffalu added 2 commits June 26, 2023 16:21

Ensure that uncommitted transactions are always removed from QoS

edea546

update comment

79ac60c

buffalu mentioned this pull request Jun 26, 2023

Feature Gate: Validator applies cost tracker to blocks during replay #29595

Open

mergify bot added community Community contribution need:merge-assist labels Jun 26, 2023

mergify bot requested a review from a team June 26, 2023 21:30

apfitzge reviewed Jun 26, 2023

View reviewed changes

core/src/banking_stage/consumer.rs Outdated Show resolved Hide resolved

pr feedback

62195d4

apfitzge requested a review from tao-stones June 26, 2023 23:02

apfitzge reviewed Jun 26, 2023

View reviewed changes

tao-stones reviewed Jun 27, 2023

View reviewed changes

core/src/banking_stage/consumer.rs Show resolved Hide resolved

core/src/banking_stage/consumer.rs Outdated Show resolved Hide resolved

core/src/banking_stage/consumer.rs Show resolved Hide resolved

core/src/banking_stage/qos_service.rs Show resolved Hide resolved

tao-stones mentioned this pull request Jun 27, 2023

Remove leader QoS adjustment logic #31379

Closed

add tests for feature on + off

a9239f1

Merge branch 'master' into lb/remove_unexecuted_txs_cost_model

f5e9c83

tao-stones previously approved these changes Jun 27, 2023

View reviewed changes

cargo fmt

4133f26

buffalu dismissed tao-stones’s stale review via 4133f26 June 28, 2023 01:23

tao-stones added the CI Pull Request is ready to enter CI label Jun 28, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Jun 28, 2023

apfitzge reviewed Jun 28, 2023

View reviewed changes

misunderstanding the logic?

8a234be

buffalu commented Jun 28, 2023

View reviewed changes

core/src/banking_stage/consumer.rs Outdated Show resolved Hide resolved

buffalu added 3 commits June 28, 2023 13:42

fix test

58369f0

remove print

905ce98

cleanup comments

188ae5c

apfitzge added the CI Pull Request is ready to enter CI label Jun 28, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Jun 28, 2023

tao-stones reviewed Jun 28, 2023

View reviewed changes

core/src/banking_stage/qos_service.rs Outdated Show resolved Hide resolved

Update core/src/banking_stage/qos_service.rs

cc1c8b4

Co-authored-by: Tao Zhu <[email protected]>

apfitzge approved these changes Jun 28, 2023

View reviewed changes

apfitzge added the CI Pull Request is ready to enter CI label Jun 28, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Jun 28, 2023

apfitzge merged commit 5dee2e4 into solana-labs:master Jun 28, 2023

tao-stones added the v1.16 PRs that should be backported to v1.16 label Jun 28, 2023

mergify bot pushed a commit that referenced this pull request Jun 28, 2023

Ensure that uncommitted transactions are always removed from QoS (#32285

2fc1f93

) Co-authored-by: Tao Zhu <[email protected]> (cherry picked from commit 5dee2e4)

mergify bot mentioned this pull request Jun 28, 2023

v1.16: Ensure that uncommitted transactions are always removed from QoS (backport of #32285) #32320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that uncommitted transactions are always removed from QoS #32285

Ensure that uncommitted transactions are always removed from QoS #32285

buffalu commented Jun 26, 2023 •

edited

Loading

apfitzge left a comment

apfitzge left a comment

tao-stones left a comment

buffalu commented Jun 27, 2023

tao-stones left a comment

buffalu commented Jun 28, 2023

apfitzge Jun 28, 2023

apfitzge commented Jun 28, 2023

buffalu commented Jun 28, 2023

buffalu commented Jun 28, 2023

tao-stones left a comment

codecov bot commented Jun 28, 2023 •

edited

Loading

buffalu commented Jun 28, 2023

apfitzge left a comment

buffalu commented Jun 28, 2023

buffalu commented Jun 28, 2023

		@@ -155,15 +166,30 @@ fn bench_process_and_record_transactions(bencher: &mut Bencher, batch_size: usiz

		#[bench]

Ensure that uncommitted transactions are always removed from QoS #32285

Ensure that uncommitted transactions are always removed from QoS #32285

Conversation

buffalu commented Jun 26, 2023 • edited Loading

Problem

Summary of Changes

apfitzge left a comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

tao-stones left a comment

Choose a reason for hiding this comment

buffalu commented Jun 27, 2023

tao-stones left a comment

Choose a reason for hiding this comment

buffalu commented Jun 28, 2023

apfitzge Jun 28, 2023

Choose a reason for hiding this comment

apfitzge commented Jun 28, 2023

buffalu commented Jun 28, 2023

buffalu commented Jun 28, 2023

tao-stones left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 28, 2023 • edited Loading

Codecov Report

buffalu commented Jun 28, 2023

apfitzge left a comment

Choose a reason for hiding this comment

buffalu commented Jun 28, 2023

buffalu commented Jun 28, 2023

buffalu commented Jun 26, 2023 •

edited

Loading

codecov bot commented Jun 28, 2023 •

edited

Loading