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

fix: prune oldest slots first in aggregator tracker #6668

Merged
merged 6 commits into from
Apr 15, 2024
Merged

fix: prune oldest slots first in aggregator tracker #6668

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dd2359f
fix: prune oldest slots first in aggregator tracker
nflaig Apr 12, 2024
a0a9c6d
Add unit tests for aggregator tracker
nflaig Apr 13, 2024
df7fdbb
Add getter to access MAX_SLOTS_CACHED from aggregator tracker
nflaig Apr 13, 2024
eadb4ab
Prune after batch of subscriptions is processed
nflaig Apr 13, 2024
564e01c
Add comment about pruning strategy
nflaig Apr 15, 2024
3022480
Tweak comment
nflaig Apr 15, 2024
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
8 changes: 6 additions & 2 deletions packages/beacon-node/src/network/processor/aggregatorTracker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,12 @@ export class AggregatorTracker {

addAggregator(subnet: SubnetId, slot: Slot): void {
this.subnetAggregatorsBySlot.getOrDefault(slot).add(subnet);

pruneSetToMax(this.subnetAggregatorsBySlot, MAX_SLOTS_CACHED);
pruneSetToMax(
this.subnetAggregatorsBySlot,
Copy link
Contributor

Choose a reason for hiding this comment

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

The alternative could be to keep subnetAggregatorsBySlot sorted at insertion time? We might not have the necessary data structure yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had three options in mind

  • sort keys when pruning (as implemented)
  • switch to pruneBySlot logic, I am not sure if this strategy is correct here, although it could keep the cache size lower, maybe @tuyennhv has some thoughts
  • as you suggested @jeluard always have keys sorted, I think we wanna keep a map for quick lookup times, although might not matter much since the array would have max 64 items

Copy link
Contributor

Choose a reason for hiding this comment

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

Current approach is probably fine given the sizes involved, probably better not to over engineer yet :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I overlooked this yesterday, we have a ordered map

lodestar/packages/beacon-node/src/util/map.ts

Line 6 in ae984f0

export class OrderedMap<K, V> {

This one keeps track of the keys which is the tradeoff I guess

Copy link
Member Author

@nflaig nflaig Apr 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Alternative using an ordered map unstable...nflaig/ordered-map-aggregator-tracker

Copy link
Member Author

Choose a reason for hiding this comment

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

turns out our "ordered" map isn't actually ordered... added units tests now

Copy link
Contributor

Choose a reason for hiding this comment

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

Current approach is probably fine given the sizes involved, probably better not to over engineer yet :)

I agree with this considering the cache size is just 64, may want to add a note for the reasoning we choose this approach

MAX_SLOTS_CACHED,
// Prune the oldest slots first
(a, b) => a - b
);
}

shouldAggregate(subnet: SubnetId, slot: Slot): boolean {
Expand Down
11 changes: 9 additions & 2 deletions packages/utils/src/map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,20 @@ export class Map2dArr<K1, V> {
/**
* Prune an arbitrary set removing the first keys to have a set.size === maxItems.
* Returns the count of deleted items.
*
* Keys can be sorted by `compareFn` to get more control over which items to prune first
*/
export function pruneSetToMax<T>(set: Set<T> | Map<T, unknown>, maxItems: number): number {
export function pruneSetToMax<T>(
set: Set<T> | Map<T, unknown>,
maxItems: number,
compareFn?: (a: T, b: T) => number
): number {
let itemsToDelete = set.size - maxItems;
const deletedItems = Math.max(0, itemsToDelete);

if (itemsToDelete > 0) {
for (const key of set.keys()) {
const keys = compareFn ? Array.from(set.keys()).sort(compareFn) : set.keys();
for (const key of keys) {
set.delete(key);
itemsToDelete--;
if (itemsToDelete <= 0) {
Expand Down
Loading