Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

replace function with const var for better readability #19285

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

tao-stones
Copy link
Contributor

Problem

const fn foo() isn't as readable as const FOO

Summary of Changes

To use const VAR, no functional change

Fixes #

@codecov
Copy link

codecov bot commented Aug 19, 2021

Codecov Report

Merging #19285 (00ed438) into master (3945908) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #19285     +/-   ##
=========================================
- Coverage    82.9%    82.8%   -0.1%     
=========================================
  Files         458      457      -1     
  Lines      130813   130793     -20     
=========================================
- Hits       108461   108365     -96     
- Misses      22352    22428     +76     

@tao-stones tao-stones requested a review from steviez August 19, 2021 15:47
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

For the sake of capturing our quick chat the other day (and partially for confirming my understanding) ...

A couple chunks from the Rust reference,

Certain forms of expressions, called constant expressions, can be evaluated at compile time. In const contexts, these are the only allowed expressions, and are always evaluated at compile time. In other places, such as let statements, constant expressions may be, but are not guaranteed to be, evaluated at compile time

The following expressions are constant expressions, so long as any operands are also constant expressions and do not cause any Drop::drop calls to be run.

  • Literals.
  • ...
  • Built-in negation, arithmetic, logical, comparison or lazy boolean operators used on integer and floating point types, bool, and char.

A const context is one of the following:
...

  • The initializer of
    • constants

A const fn is a function that one is permitted to call from a const context ... When called from a const context, the function is interpreted by the compiler at compile time

So my understanding is that:

  • Defining a constant (ie pub const seven = 3 + 4) is a const context (constant initializer), and this is guaranteed to be evaluated at compile-time.
  • A const fn can be used in a const context, in which case it would be evaluated at compile time.
  • A const fn can also be used in a non-const context, in which case it may be evaluated at compile time.

Given the above, I don't think the const fn's would be guaranteed to be resolved at compile-time (although probably likely would) since they aren't all used in const contexts, so using constants gives us that guarantee too.

Interested to hear your thought, but regardless, change looks good to me!

@tao-stones
Copy link
Contributor Author

tao-stones commented Aug 19, 2021

For the sake of capturing our quick chat the other day (and partially for confirming my understanding) ...

A couple chunks from the Rust reference,

Certain forms of expressions, called constant expressions, can be evaluated at compile time. In const contexts, these are the only allowed expressions, and are always evaluated at compile time. In other places, such as let statements, constant expressions may be, but are not guaranteed to be, evaluated at compile time

The following expressions are constant expressions, so long as any operands are also constant expressions and do not cause any Drop::drop calls to be run.

  • Literals.
  • ...
  • Built-in negation, arithmetic, logical, comparison or lazy boolean operators used on integer and floating point types, bool, and char.

A const context is one of the following:
...

  • The initializer of

    • constants

A const fn is a function that one is permitted to call from a const context ... When called from a const context, the function is interpreted by the compiler at compile time

So my understanding is that:

  • Defining a constant (ie pub const seven = 3 + 4) is a const context (constant initializer), and this is guaranteed to be evaluated at compile-time.
  • A const fn can be used in a const context, in which case it would be evaluated at compile time.
  • A const fn can also be used in a non-const context, in which case it may be evaluated at compile time.

Given the above, I don't think the const fn's would be guaranteed to be resolved at compile-time (although probably likely would) since they aren't all used in const contexts, so using constants gives us that guarantee too.

Interested to hear your thought, but regardless, change looks good to me!

Nice summary! I think it is within const context in my use of these const function. The performance tests I did before/after suggests this is the case. But honestly, I am not sure what does non-const context mean, as in const fn(variable: &mut u64) {}?

@steviez
Copy link
Contributor

steviez commented Aug 19, 2021

Nice summary! I think it is within const context in my use of these const function. The performance tests I did before/after suggests this is the case. But honestly, I am not sure what does non-const context mean, as in const fn(variable: &mut u64) {}?

Gotcha - I think I was mixed up on the which context is relevant here. Originally, I was thinking the caller of the constant/const function was the one that mattered. Such as:

self.transaction_cost.account_access_cost += ACCOUNT_READ_COST; // or account_read_cost()

But, as you pointed out to me in DM, I think the context of the const function itself is probably what is relevant.

pub const fn account_read_cost() -> u64 {
    // read account averages 5us
    compute_unit_to_us_ratio() * 5
}

This makes sense as the caller of a function shouldn't dictate how / when the compiler evaluates that function. Your quick test agrees with this so I'm all good / think I'm straightened out ... enough of this rabbit hole and back to real things ha

@tao-stones tao-stones merged commit 4982dc2 into solana-labs:master Aug 19, 2021
tao-stones added a commit to tao-stones/solana that referenced this pull request Sep 24, 2021
tao-stones added a commit to tao-stones/solana that referenced this pull request Sep 24, 2021
tao-stones added a commit to tao-stones/solana that referenced this pull request Sep 29, 2021
tao-stones added a commit to tao-stones/solana that referenced this pull request Sep 30, 2021
tao-stones added a commit to tao-stones/solana that referenced this pull request Oct 6, 2021
tao-stones added a commit that referenced this pull request Oct 6, 2021
* Cost Model to limit transactions which are not parallelizeable (#16694)

* * Add following to banking_stage:
  1. CostModel as immutable ref shared between threads, to provide estimated cost for transactions.
  2. CostTracker which is shared between threads, tracks transaction costs for each block.

* replace hard coded program ID with id() calls

* Add Account Access Cost as part of TransactionCost. Account Access cost are weighted differently between read and write, signed and non-signed.

* Establish instruction_execution_cost_table, add function to update or insert instruction cost, unit tested. It is read-only for now; it allows Replay to insert realtime instruction execution costs to the table.

* add test for cost_tracker atomically try_add operation, serves as safety guard for future changes

* check cost against local copy of cost_tracker, return transactions that would exceed limit as unprocessed transaction to be buffered; only apply bank processed transactions cost to tracker;

* bencher to new banking_stage with max cost limit to allow cost model being hit consistently during bench iterations

* replay stage feed back program cost (#17731)

* replay stage feeds back realtime per-program execution cost to cost model;

* program cost execution table is initialized into empty table, no longer populated with hardcoded numbers;

* changed cost unit to microsecond, using value collected from mainnet;

* add ExecuteCostTable with fixed capacity for security concern, when its limit is reached, programs with old age AND less occurrence will be pushed out to make room for new programs.

* investigate system performance test degradation  (#17919)

* Add stats and counter around cost model ops, mainly:
- calculate transaction cost
- check transaction can fit in a block
- update block cost tracker after transactions are added to block
- replay_stage to update/insert execution cost to table

* Change mutex on cost_tracker to RwLock

* removed cloning cost_tracker for local use, as the metrics show clone is very expensive.

* acquire and hold locks for block of TXs, instead of acquire and release per transaction;

* remove redundant would_fit check from cost_tracker update execution path

* refactor cost checking with less frequent lock acquiring

* avoid many Transaction_cost heap allocation when calculate cost, which
is in the hot path - executed per transaction.

* create hashmap with new_capacity to reduce runtime heap realloc.

* code review changes: categorize stats, replace explicit drop calls, concisely initiate to default

* address potential deadlock by acquiring locks one at time

* Persist cost table to blockstore (#18123)

* Add `ProgramCosts` Column Family to blockstore, implement LedgerColumn; add `delete_cf` to Rocks
* Add ProgramCosts to compaction excluding list alone side with TransactionStatusIndex in one place: `excludes_from_compaction()`

* Write cost table to blockstore after `replay_stage` replayed active banks; add stats to measure persist time
* Deletes program from `ProgramCosts` in blockstore when they are removed from cost_table in memory
* Only try to persist to blockstore when cost_table is changed.
* Restore cost table during validator startup

* Offload `cost_model` related operations from replay main thread to dedicated service thread, add channel to send execute_timings between these threads;
* Move `cost_update_service` to its own module; replay_stage is now decoupled from cost_model.

* log warning when channel send fails (#18391)

* Aggregate cost_model into cost_tracker (#18374)

* * aggregate cost_model into cost_tracker, decouple it from banking_stage to prevent accidental deadlock. * Simplified code, removed unused functions

* review fixes

* update ledger tool to restore cost table from blockstore (#18489)

* update ledger tool to restore cost model from blockstore when compute-slot-cost

* Move initialize_cost_table into cost_model, so the function can be tested and shared between validator and ledger-tool

* refactor and simplify a test

* manually fix merge conflicts

* Per-program id timings (#17554)

* more manual fixing

* solve a merge conflict

* featurize cost model

* more merge fix

* cost model uses compute_unit to replace microsecond as cost unit
(#18934)

* Reject blocks for costs above the max block cost (#18994)

* Update block max cost limit to fix performance regession (#19276)

* replace function with const var for better readability (#19285)

* Add few more metrics data points (#19624)

* periodically report sigverify_stage stats (#19674)

* manual merge

* cost model nits (#18528)

* Accumulate consumed units (#18714)

* tx wide compute budget (#18631)

* more manual merge

* ignore zerorize drop security

* - update const cost values with data collected by #19627
- update cost calculation to closely proposed fee schedule #16984

* add transaction cost histogram metrics (#20350)

* rebase to 1.7.15

* add tx count and thread id to stats (#20451)
each stat reports and resets when slot changes

* remove cost_model feature_set

* ignore vote transactions from cost model

Co-authored-by: sakridge <[email protected]>
Co-authored-by: Jeff Biseda <[email protected]>
Co-authored-by: Jack May <[email protected]>
t-nelson pushed a commit that referenced this pull request Oct 6, 2021
* Cost Model to limit transactions which are not parallelizeable (#16694)

* * Add following to banking_stage:
  1. CostModel as immutable ref shared between threads, to provide estimated cost for transactions.
  2. CostTracker which is shared between threads, tracks transaction costs for each block.

* replace hard coded program ID with id() calls

* Add Account Access Cost as part of TransactionCost. Account Access cost are weighted differently between read and write, signed and non-signed.

* Establish instruction_execution_cost_table, add function to update or insert instruction cost, unit tested. It is read-only for now; it allows Replay to insert realtime instruction execution costs to the table.

* add test for cost_tracker atomically try_add operation, serves as safety guard for future changes

* check cost against local copy of cost_tracker, return transactions that would exceed limit as unprocessed transaction to be buffered; only apply bank processed transactions cost to tracker;

* bencher to new banking_stage with max cost limit to allow cost model being hit consistently during bench iterations

* replay stage feed back program cost (#17731)

* replay stage feeds back realtime per-program execution cost to cost model;

* program cost execution table is initialized into empty table, no longer populated with hardcoded numbers;

* changed cost unit to microsecond, using value collected from mainnet;

* add ExecuteCostTable with fixed capacity for security concern, when its limit is reached, programs with old age AND less occurrence will be pushed out to make room for new programs.

* investigate system performance test degradation  (#17919)

* Add stats and counter around cost model ops, mainly:
- calculate transaction cost
- check transaction can fit in a block
- update block cost tracker after transactions are added to block
- replay_stage to update/insert execution cost to table

* Change mutex on cost_tracker to RwLock

* removed cloning cost_tracker for local use, as the metrics show clone is very expensive.

* acquire and hold locks for block of TXs, instead of acquire and release per transaction;

* remove redundant would_fit check from cost_tracker update execution path

* refactor cost checking with less frequent lock acquiring

* avoid many Transaction_cost heap allocation when calculate cost, which
is in the hot path - executed per transaction.

* create hashmap with new_capacity to reduce runtime heap realloc.

* code review changes: categorize stats, replace explicit drop calls, concisely initiate to default

* address potential deadlock by acquiring locks one at time

* Persist cost table to blockstore (#18123)

* Add `ProgramCosts` Column Family to blockstore, implement LedgerColumn; add `delete_cf` to Rocks
* Add ProgramCosts to compaction excluding list alone side with TransactionStatusIndex in one place: `excludes_from_compaction()`

* Write cost table to blockstore after `replay_stage` replayed active banks; add stats to measure persist time
* Deletes program from `ProgramCosts` in blockstore when they are removed from cost_table in memory
* Only try to persist to blockstore when cost_table is changed.
* Restore cost table during validator startup

* Offload `cost_model` related operations from replay main thread to dedicated service thread, add channel to send execute_timings between these threads;
* Move `cost_update_service` to its own module; replay_stage is now decoupled from cost_model.

* log warning when channel send fails (#18391)

* Aggregate cost_model into cost_tracker (#18374)

* * aggregate cost_model into cost_tracker, decouple it from banking_stage to prevent accidental deadlock. * Simplified code, removed unused functions

* review fixes

* update ledger tool to restore cost table from blockstore (#18489)

* update ledger tool to restore cost model from blockstore when compute-slot-cost

* Move initialize_cost_table into cost_model, so the function can be tested and shared between validator and ledger-tool

* refactor and simplify a test

* manually fix merge conflicts

* Per-program id timings (#17554)

* more manual fixing

* solve a merge conflict

* featurize cost model

* more merge fix

* cost model uses compute_unit to replace microsecond as cost unit
(#18934)

* Reject blocks for costs above the max block cost (#18994)

* Update block max cost limit to fix performance regession (#19276)

* replace function with const var for better readability (#19285)

* Add few more metrics data points (#19624)

* periodically report sigverify_stage stats (#19674)

* manual merge

* cost model nits (#18528)

* Accumulate consumed units (#18714)

* tx wide compute budget (#18631)

* more manual merge

* ignore zerorize drop security

* - update const cost values with data collected by #19627
- update cost calculation to closely proposed fee schedule #16984

* add transaction cost histogram metrics (#20350)

* rebase to 1.7.15

* add tx count and thread id to stats (#20451)
each stat reports and resets when slot changes

* remove cost_model feature_set

* ignore vote transactions from cost model

Co-authored-by: sakridge <[email protected]>
Co-authored-by: Jeff Biseda <[email protected]>
Co-authored-by: Jack May <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants