Skip to content

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented Mar 24, 2025

  • Configurable dirty node shard count.
  • So that unit test can run with very little shard and I guess some weird setup with very high core count can have better parallelism during pruning.

Changes

  • Configurable dirty node shard count by configuring shard bit.

Types of changes

What types of changes does your code introduce?

  • New feature (a non-breaking change that adds functionality)

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

  • Seems to work.

@asdacap asdacap requested a review from rubo as a code owner March 24, 2025 17:08
Comment on lines +86 to +89
if (_shardBit is <= 0 or > 30)
{
throw new InvalidOperationException($"Shard bit count must be between 0 and 30.");
}
Copy link
Member

Choose a reason for hiding this comment

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

Move this validation directly to config class?

: BinaryPrimitives.ReadUInt32BigEndian(hash.Bytes[..4]);

// Using some early bits so that nodes of similar prefix would get put in the same shard.
uint shardIdx = baseSize >> (32 - _shardBit);
Copy link
Member

Choose a reason for hiding this comment

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

32 - _shardBit could be saved in a field.

Copy link
Member

@benaadams benaadams left a comment

Choose a reason for hiding this comment

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

Change to use sampling from GetHashCode() so similar paths don't sample together

Co-authored-by: Ben {chmark} Adams <thundercat@illyriad.co.uk>
@@ -79,4 +79,7 @@ The max number of parallel tasks that can be used by full pruning.

[ConfigItem(Description = "The number of past states before the state gets pruned. Used to determine how old of a state to keep from the head.", DefaultValue = "64")]
int PruningBoundary { get; set; }

[ConfigItem(Description = "Dirty node shard count", DefaultValue = "8")]
int DirtyNodeShardBit { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it default to something based on CPU count maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, one day. But for now I can't find any difference unless it is set too low where pruning can't parallelize.

asdacap and others added 2 commits March 25, 2025 20:16
Co-authored-by: Ben {chmark} Adams <thundercat@illyriad.co.uk>
Co-authored-by: Ben {chmark} Adams <thundercat@illyriad.co.uk>
@benaadams
Copy link
Member

Looks like failure is persistent

Failed Retain_Some_PersistedNodes [185 ms]
  Error Message:
   Expected value to be 324L, but found 325L (difference of 1).

@asdacap asdacap merged commit 90ff39f into master Mar 25, 2025
80 checks passed
@asdacap asdacap deleted the feature/configurable-shard-count branch March 25, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants