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

Clone states only when necessary #4279

Merged
merged 4 commits into from
Jul 18, 2022
Merged

Clone states only when necessary #4279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
309c973
Clone states only when necessary
dapplion Jul 11, 2022
5347c3c
Use validators.getReadonly when not need mutable reference
dapplion Jul 11, 2022
023ae85
Merge branch 'unstable' into dapplion/clone-when-necessary
dapplion Jul 12, 2022
c08477e
Merge branch 'unstable' into dapplion/clone-when-necessary
dapplion Jul 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/beacon-node/src/api/impl/beacon/state/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export function getBeaconStateApi({
for (const id of filters.id) {
const validatorIndex = getStateValidatorIndex(id, state, pubkey2index);
if (validatorIndex != null) {
const validator = validators.get(validatorIndex);
const validator = validators.getReadonly(validatorIndex);
if (filters.statuses && !filters.statuses.includes(getValidatorStatus(validator, currentEpoch))) {
continue;
}
Expand Down Expand Up @@ -99,7 +99,7 @@ export function getBeaconStateApi({
return {
data: toValidatorResponse(
validatorIndex,
state.validators.get(validatorIndex),
state.validators.getReadonly(validatorIndex),
state.balances.get(validatorIndex),
getCurrentEpoch(state)
),
Expand Down
2 changes: 1 addition & 1 deletion packages/beacon-node/src/chain/blocks/importBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export async function importBlock(
// - Write block and state to snapshot_cache
if (block.message.slot % SLOTS_PER_EPOCH === 0) {
// Cache state to preserve epoch transition work
const checkpointState = postState.clone();
const checkpointState = postState;
const cp = getCheckpointFromState(checkpointState);
chain.checkpointStateCache.add(cp, checkpointState);
pendingEvents.push(ChainEvent.checkpoint, cp, checkpointState);
Expand Down
8 changes: 4 additions & 4 deletions packages/beacon-node/src/chain/opPools/opPool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export class OpPool {

for (const proposerSlashing of this.proposerSlashings.values()) {
const index = proposerSlashing.signedHeader1.message.proposerIndex;
const validator = state.validators.get(index);
const validator = state.validators.getReadonly(index);
if (!validator.slashed && validator.activationEpoch <= stateEpoch && stateEpoch < validator.withdrawableEpoch) {
proposerSlashings.push(proposerSlashing);
// Set of validators to be slashed, so we don't attempt to construct invalid attester slashings.
Expand All @@ -156,7 +156,7 @@ export class OpPool {
const slashableIndices = new Set<ValidatorIndex>();
for (let i = 0; i < attesterSlashing.intersectingIndices.length; i++) {
const index = attesterSlashing.intersectingIndices[i];
const validator = state.validators.get(index);
const validator = state.validators.getReadonly(index);

// If we already have a slashing for this index, we can continue on to the next slashing
if (toBeSlashedIndices.has(index)) {
Expand Down Expand Up @@ -232,7 +232,7 @@ export class OpPool {
//
// We cannot check the `slashed` field since the `head` is not finalized and
// a fork could un-slash someone.
if (headState.validators.get(index).exitEpoch > finalizedEpoch) {
if (headState.validators.getReadonly(index).exitEpoch > finalizedEpoch) {
continue attesterSlashing;
}
}
Expand All @@ -249,7 +249,7 @@ export class OpPool {
const finalizedEpoch = headState.finalizedCheckpoint.epoch;
for (const [key, proposerSlashing] of this.proposerSlashings.entries()) {
const index = proposerSlashing.signedHeader1.message.proposerIndex;
if (headState.validators.get(index).exitEpoch <= finalizedEpoch) {
if (headState.validators.getReadonly(index).exitEpoch <= finalizedEpoch) {
this.proposerSlashings.delete(key);
}
}
Expand Down
5 changes: 3 additions & 2 deletions packages/beacon-node/src/chain/regen/regen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,18 +242,19 @@ async function processSlotsToNearestCheckpoint(
const preSlot = preState.slot;
const postSlot = slot;
const preEpoch = computeEpochAtSlot(preSlot);
let postState = preState.clone();
let postState = preState;
const {checkpointStateCache, emitter, metrics} = modules;

for (
let nextEpochSlot = computeStartSlotAtEpoch(preEpoch + 1);
nextEpochSlot <= postSlot;
nextEpochSlot += SLOTS_PER_EPOCH
) {
// processSlots calls .clone() before mutating
postState = processSlots(postState, nextEpochSlot, {}, metrics);

// Cache state to preserve epoch transition work
const checkpointState = postState.clone();
const checkpointState = postState;
Copy link
Contributor

Choose a reason for hiding this comment

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

@dapplion I think it's safer to do postState.clone(false)?

from your implementation

  clone(dontTransferCache?: boolean): this {
    if (dontTransferCache) {
      return this.type.getViewDU(this.node) as this;
    } else {
      const cache = this.cache;
      this.clearCache();
      return this.type.getViewDU(this.node, cache) as this;
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don't follow, what would it be safer to do a clone here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tuyennhv The point of this PR is to think critically about when our state can actually be mutated. My opinion is that we clone is too many places that a state can't be mutated so cloning is an unnecessary expense that slows than Lodestar for no gain

Copy link
Contributor

Choose a reason for hiding this comment

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

@dapplion in the past we used to have an issue of missing a clone() call, it really took time to debug in that case, @wemeetagain may experience it

so what I mean is instead of dropping the clone() call, can we do clone(false) with less performance impact?

if both clone() and clone(false) cause performance issue then we need to benchmark to improve performance?

Copy link
Contributor

Choose a reason for hiding this comment

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

My confusion: I thought the cache is merely used for the mutation, it also contains the index of nodes so that we only have to traverse to the node once.

if we use clone(false), the consumer does not have a chance to use the cached nodes of the tree so it's all bad going with either clone() or clone(false)

with ssz v2, we should not ever have a state mutation unless in state transition, thanks @dapplion for your explanation 👍

const cp = getCheckpointFromState(checkpointState);
checkpointStateCache.add(cp, checkpointState);
emitter.emit(ChainEvent.checkpoint, cp, checkpointState);
Expand Down
13 changes: 2 additions & 11 deletions packages/beacon-node/src/chain/stateCache/stateContextCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,21 +41,12 @@ export class StateContextCache {
}

this.metrics?.hits.inc();
// clonedCount + 1 as there's a .clone() below
this.metrics?.stateClonedCount.observe(item.clonedCount + 1);
if (!stateInternalCachePopulated(item)) {
this.metrics?.stateInternalCacheMiss.inc();
}

// Clone first to account for metrics below
const itemCloned = item.clone();

this.metrics?.stateClonedCount.observe(item.clonedCount);
Copy link
Contributor

Choose a reason for hiding this comment

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

since we don't call clone() anymore, should metric observe clonedCount here?

if (!stateInternalCachePopulated(item)) {
this.metrics?.stateInternalCacheMiss.inc();
}

return itemCloned;
return item;
}

add(item: CachedBeaconStateAllForks): void {
Expand All @@ -64,7 +55,7 @@ export class StateContextCache {
return;
}
this.metrics?.adds.inc();
this.cache.set(key, item.clone());
this.cache.set(key, item);
const epoch = item.epochCtx.epoch;
const blockRoots = this.epochIndex.get(epoch);
if (blockRoots) {
Expand Down
7 changes: 2 additions & 5 deletions packages/beacon-node/src/chain/stateCache/stateContextCheckpointsCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,12 @@ export class CheckpointStateCache {
this.preComputedCheckpointHits = (this.preComputedCheckpointHits ?? 0) + 1;
}

// Clone first to account for metrics below
const itemCloned = item.clone();

this.metrics?.stateClonedCount.observe(item.clonedCount);
if (!stateInternalCachePopulated(item)) {
this.metrics?.stateInternalCacheMiss.inc();
}

return itemCloned;
return item;
}

add(cp: phase0.Checkpoint, item: CachedBeaconStateAllForks): void {
Expand All @@ -65,7 +62,7 @@ export class CheckpointStateCache {
return;
}
this.metrics?.adds.inc();
this.cache.set(key, item.clone());
this.cache.set(key, item);
this.epochIndex.getOrDefault(cp.epoch).add(cpHex.rootHex);
}

Expand Down
8 changes: 6 additions & 2 deletions packages/state-transition/src/block/initiateValidatorExit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {FAR_FUTURE_EPOCH} from "@lodestar/params";
import {phase0} from "@lodestar/types";
import {CompositeViewDU} from "@chainsafe/ssz";
import {ssz} from "@lodestar/types";
import {CachedBeaconStateAllForks} from "../types.js";

/**
Expand All @@ -22,7 +23,10 @@ import {CachedBeaconStateAllForks} from "../types.js";
* ```
* Forcing consumers to pass the SubTree of `validator` directly mitigates this issue.
*/
export function initiateValidatorExit(state: CachedBeaconStateAllForks, validator: phase0.Validator): void {
export function initiateValidatorExit(
state: CachedBeaconStateAllForks,
validator: CompositeViewDU<typeof ssz.phase0.Validator>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using CompositeViewDU to express the need for mutability here

): void {
const {config, epochCtx} = state;

// return if validator already initiated exit
Expand Down
2 changes: 1 addition & 1 deletion packages/state-transition/src/block/processAttesterSlashing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function processAttesterSlashing(
const validators = state.validators; // Get the validators sub tree once for all indices
// Spec requires to sort indexes beforehand
for (const index of intersectingIndices.sort((a, b) => a - b)) {
if (isSlashableValidator(validators.get(index), state.epochCtx.epoch)) {
if (isSlashableValidator(validators.getReadonly(index), state.epochCtx.epoch)) {
slashValidator(fork, state, index);
slashedAny = true;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/state-transition/src/block/processBlockHeader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function processBlockHeader(state: CachedBeaconStateAllForks, block: allF
});

// verify proposer is not slashed. Only once per block, may use the slower read from tree
if (state.validators.get(proposerIndex).slashed) {
if (state.validators.getReadonly(proposerIndex).slashed) {
throw new Error("Block proposer is slashed");
}
}
2 changes: 1 addition & 1 deletion packages/state-transition/src/block/processProposerSlashing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export function assertValidProposerSlashing(
}

// verify the proposer is slashable
const proposer = state.validators.get(header1.proposerIndex);
const proposer = state.validators.getReadonly(header1.proposerIndex);
if (!isSlashableValidator(proposer, state.epochCtx.epoch)) {
throw new Error("ProposerSlashing proposer is not slashable");
}
Expand Down
4 changes: 3 additions & 1 deletion packages/state-transition/src/epoch/processSyncCommitteeUpdates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ export function processSyncCommitteeUpdates(state: CachedBeaconStateAltair): voi
);

// Using the index2pubkey cache is slower because it needs the serialized pubkey.
const nextSyncCommitteePubkeys = nextSyncCommitteeIndices.map((index) => state.validators.get(index).pubkey);
const nextSyncCommitteePubkeys = nextSyncCommitteeIndices.map(
(index) => state.validators.getReadonly(index).pubkey
);

// Rotate syncCommittee in state
state.currentSyncCommittee = state.nextSyncCommittee;
Expand Down
2 changes: 2 additions & 0 deletions packages/state-transition/src/stateTransition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export function stateTransition(
const block = signedBlock.message;
const blockSlot = block.slot;

// .clone() before mutating state in state transition
let postState = state.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here and line 85 are the only two instances where state is cloned


// State is already a ViewDU, which won't commit changes. Equivalent to .setStateCachesAsTransient()
Expand Down Expand Up @@ -88,6 +89,7 @@ export function processSlots(
epochProcessOpts?: EpochProcessOpts,
metrics?: IBeaconStateTransitionMetrics | null
): CachedBeaconStateAllForks {
// .clone() before mutating state in state transition
let postState = state.clone();

// State is already a ViewDU, which won't commit changes. Equivalent to .setStateCachesAsTransient()
Expand Down
2 changes: 1 addition & 1 deletion packages/state-transition/src/util/syncCommittee.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function getNextSyncCommittee(
const indices = getNextSyncCommitteeIndices(state, activeValidatorIndices, effectiveBalanceIncrements);

// Using the index2pubkey cache is slower because it needs the serialized pubkey.
const pubkeys = indices.map((index) => state.validators.get(index).pubkey);
const pubkeys = indices.map((index) => state.validators.getReadonly(index).pubkey);

return {
indices,
Expand Down
2 changes: 1 addition & 1 deletion packages/state-transition/test/perf/analyzeEpochs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ async function analyzeEpochs(network: NetworkName, fromEpoch?: number): Promise<
const validatorKeys = Object.keys(validatorChangesCountZero) as (keyof typeof validatorChangesCountZero)[];
for (let i = 0; i < validatorCount; i++) {
const validatorPrev = state.validators[i];
const validatorNext = postState.validators.get(i);
const validatorNext = postState.validators.getReadonly(i);
for (const key of validatorKeys) {
const valuePrev = validatorPrev[key];
const valueNext = validatorNext[key];
Expand Down