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.

Sign up for GitHub

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

Jump to bottom

Demote write locks on transaction program ids #19593

Merged
merged 6 commits into from
Sep 4, 2021
Merged

Demote write locks on transaction program ids #19593

merged 6 commits into from
Sep 4, 2021

Conversation

CriesofCarrots
Copy link
Contributor

Problem

Programs can't be written to, and taking a write lock on them can make batch sizes smaller than they should be.

Summary of Changes

Lower write lock to read lock for accounts marked as program-ids in a transaction.

@CriesofCarrots
Copy link
Contributor Author

@taozhu-chicago would you mind sanity checking the banking_stage to cost_model changes?

@jackcmay
Copy link
Contributor

jackcmay commented Sep 2, 2021

Be nice to also demote write locks by default in tx/message/instruction construction

@CriesofCarrots
Copy link
Contributor Author

CriesofCarrots commented Sep 2, 2021 •
edited
Loading

Be nice to also demote write locks by default in tx/message/instruction construction

Good idea, I'll add that, once we decide on this approach or #19601

@tao-stones
Copy link
Contributor

@taozhu-chicago would you mind sanity checking the banking_stage to cost_model changes?

the changes on banking_stage/cost_model path look good to me.

tao-stones
tao-stones reviewed Sep 3, 2021
View reviewed changes
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

It does take a lot of plumbing to get to banking_stage, maybe there is another way cost_model can check for TX's write locks without diving deep into is_writable() call?

@codecov
Copy link

codecov bot commented Sep 3, 2021 •
edited
Loading

Codecov Report

Merging #19593 (ce89201) into master (c550b32) will decrease coverage by 0.1%.
The diff coverage is 51.9%.

@@            Coverage Diff            @@
##           master   #19593     +/-   ##
=========================================
- Coverage    82.7%    82.5%   -0.2%     
=========================================
  Files         461      468      +7     
  Lines      131215   131820    +605     
=========================================
+ Hits       108519   108772    +253     
- Misses      22696    23048    +352

@CriesofCarrots
Copy link
Contributor Author

It does take a lot of plumbing to get to banking_stage, maybe there is another way cost_model can check for TX's write locks without diving deep into is_writable() call?

Not a bad idea @taozhu-chicago , but no worries for this pr. We need the changes in is_writable anyway, and have easy access to the bank/feature-set.

@CriesofCarrots
Copy link
Contributor Author

Be nice to also demote write locks by default in tx/message/instruction construction

I tried this out here: CriesofCarrots@19ee2de
It's messy, and spurs the question whether we should do the same for sysvars and builtin programs (as per is_writable). So I vote no on including.

jstarry
jstarry previously approved these changes Sep 3, 2021
View reviewed changes
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

looks good to me, just a few nits

program-runtime/src/instruction_processor.rs Outdated Show resolved Hide resolved
sdk/program/src/message/sanitized.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed jstarry’s stale review September 3, 2021 19:41

Pull request has been modified.

jackcmay
jackcmay previously approved these changes Sep 3, 2021
View reviewed changes
@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Sep 3, 2021
@CriesofCarrots
Copy link
Contributor Author

Feature_set conflicts make me sad

@solana-grimes
Copy link
Contributor

😱 New commits were pushed while the automerge label was present.

@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Sep 3, 2021
@mergify mergify bot dismissed jackcmay’s stale review September 3, 2021 22:59

Pull request has been modified.

@CriesofCarrots CriesofCarrots mentioned this pull request Sep 3, 2021
Merged
@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Sep 4, 2021
@mergify mergify bot merged commit decec3c into solana-labs:master Sep 4, 2021
mergify bot pushed a commit that referenced this pull request Sep 4, 2021
@CriesofCarrots
Demote write locks on transaction program ids (#19593)
31d1857 
* Add feature

* Demote write lock on program ids

* Fixup bpf tests

* Update MappedMessage::is_writable

* Comma nit

* Review comments

(cherry picked from commit decec3c)

# Conflicts:
#	cli-output/src/display.rs
#	core/src/banking_stage.rs
#	core/src/cost_model.rs
#	core/src/cost_tracker.rs
#	core/src/transaction_status_service.rs
#	ledger-tool/src/main.rs
#	program-runtime/src/instruction_processor.rs
#	program-test/src/lib.rs
#	programs/bpf/tests/programs.rs
#	programs/bpf_loader/src/syscalls.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/message_processor.rs
#	sdk/benches/serialize_instructions.rs
#	sdk/program/src/message.rs
#	sdk/program/src/message/mapped.rs
#	sdk/program/src/message/sanitized.rs
#	sdk/src/feature_set.rs
#	sdk/src/transaction/sanitized.rs
#	transaction-status/src/parse_accounts.rs
@mergify mergify bot mentioned this pull request Sep 4, 2021
Closed
mergify bot pushed a commit that referenced this pull request Sep 4, 2021
@CriesofCarrots
Demote write locks on transaction program ids (#19593)
d40bb38 
* Add feature

* Demote write lock on program ids

* Fixup bpf tests

* Update MappedMessage::is_writable

* Comma nit

* Review comments

(cherry picked from commit decec3c)

# Conflicts:
#	core/src/banking_stage.rs
#	core/src/cost_model.rs
#	core/src/cost_tracker.rs
#	ledger-tool/src/main.rs
#	program-runtime/src/instruction_processor.rs
#	programs/bpf/tests/programs.rs
#	programs/bpf_loader/src/syscalls.rs
#	rpc/src/transaction_status_service.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/message_processor.rs
#	sdk/benches/serialize_instructions.rs
#	sdk/program/src/message/mapped.rs
#	sdk/program/src/message/sanitized.rs
#	sdk/src/transaction/sanitized.rs
@mergify mergify bot mentioned this pull request Sep 4, 2021
Merged
mergify bot added a commit that referenced this pull request Sep 4, 2021
@mergify @CriesofCarrots
Demote write locks on transaction program ids (backport #19593) (#19633)
fcda5d4 
* Demote write locks on transaction program ids (#19593)

* Add feature

* Demote write lock on program ids

* Fixup bpf tests

* Update MappedMessage::is_writable

* Comma nit

* Review comments

(cherry picked from commit decec3c)

# Conflicts:
#	core/src/banking_stage.rs
#	core/src/cost_model.rs
#	core/src/cost_tracker.rs
#	ledger-tool/src/main.rs
#	program-runtime/src/instruction_processor.rs
#	programs/bpf/tests/programs.rs
#	programs/bpf_loader/src/syscalls.rs
#	rpc/src/transaction_status_service.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/message_processor.rs
#	sdk/benches/serialize_instructions.rs
#	sdk/program/src/message/mapped.rs
#	sdk/program/src/message/sanitized.rs
#	sdk/src/transaction/sanitized.rs

* Fix conflicts

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
mergify bot added a commit that referenced this pull request Sep 4, 2021
@mergify
Demote write locks on transaction program ids (backport #19593) (#19633)
244cb2e 
* Demote write locks on transaction program ids (#19593)

* Add feature

* Demote write lock on program ids

* Fixup bpf tests

* Update MappedMessage::is_writable

* Comma nit

* Review comments

(cherry picked from commit decec3c)

# Conflicts:
#	core/src/banking_stage.rs
#	core/src/cost_model.rs
#	core/src/cost_tracker.rs
#	ledger-tool/src/main.rs
#	program-runtime/src/instruction_processor.rs
#	programs/bpf/tests/programs.rs
#	programs/bpf_loader/src/syscalls.rs
#	rpc/src/transaction_status_service.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/message_processor.rs
#	sdk/benches/serialize_instructions.rs
#	sdk/program/src/message/mapped.rs
#	sdk/program/src/message/sanitized.rs
#	sdk/src/transaction/sanitized.rs

* Fix conflicts

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
(cherry picked from commit fcda5d4)

# Conflicts:
#	cli-output/src/display.rs
#	core/src/transaction_status_service.rs
#	program-test/src/lib.rs
#	programs/bpf_loader/src/syscalls.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/message_processor.rs
#	sdk/benches/serialize_instructions.rs
#	sdk/program/src/message.rs
#	sdk/src/feature_set.rs
#	transaction-status/src/parse_accounts.rs
mergify bot added a commit that referenced this pull request Sep 4, 2021
@mergify @CriesofCarrots
Demote write locks on transaction program ids (backport #19593) (back…
53f8e58 
…port #19633) (#19637)

* Demote write locks on transaction program ids (backport #19593) (#19633)

* Demote write locks on transaction program ids (#19593)

* Add feature

* Demote write lock on program ids

* Fixup bpf tests

* Update MappedMessage::is_writable

* Comma nit

* Review comments

(cherry picked from commit decec3c)

# Conflicts:
#	core/src/banking_stage.rs
#	core/src/cost_model.rs
#	core/src/cost_tracker.rs
#	ledger-tool/src/main.rs
#	program-runtime/src/instruction_processor.rs
#	programs/bpf/tests/programs.rs
#	programs/bpf_loader/src/syscalls.rs
#	rpc/src/transaction_status_service.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/message_processor.rs
#	sdk/benches/serialize_instructions.rs
#	sdk/program/src/message/mapped.rs
#	sdk/program/src/message/sanitized.rs
#	sdk/src/transaction/sanitized.rs

* Fix conflicts

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
(cherry picked from commit fcda5d4)

# Conflicts:
#	cli-output/src/display.rs
#	core/src/transaction_status_service.rs
#	program-test/src/lib.rs
#	programs/bpf_loader/src/syscalls.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/message_processor.rs
#	sdk/benches/serialize_instructions.rs
#	sdk/program/src/message.rs
#	sdk/src/feature_set.rs
#	transaction-status/src/parse_accounts.rs

* Replace feature and fix conflicts

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
@frits-metalogix
Revert "Demote write locks on transaction program ids (solana-labs#19593
d2131ae 
)"

This reverts commit 824e9c9.
@steviez steviez mentioned this pull request Dec 18, 2021
Closed
@CriesofCarrots CriesofCarrots deleted the demote-program-locks branch July 27, 2022 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tao-stones tao-stones tao-stones left review comments

@KeKo6988 KeKo6988 KeKo6988 approved these changes

@jackcmay jackcmay jackcmay left review comments

@jstarry jstarry jstarry left review comments

Assignees
No one assigned
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@CriesofCarrots @jackcmay @tao-stones @solana-grimes @jstarry @KeKo6988