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.

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

asim: batch workload events #83109

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Jun 20, 2022

This patch introduces batching for load events. Previously, load events
were generated per-key and applied individually to the simulator state
by finding the range containing that key. This patch batches load events
on the same key, then applies the load events in ascending order, over
the range tree.

This results in a speedup of 5x on a 32 store, 32k replicas, 16k QPS
cluster.

Release note: None

@kvoli kvoli self-assigned this Jun 20, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli marked this pull request as ready for review June 21, 2022 14:33
@kvoli kvoli requested a review from a team as a code owner June 21, 2022 14:33
@kvoli kvoli requested review from lidorcarmel and removed request for a team June 21, 2022 14:33
Copy link
Contributor

@lidorcarmel lidorcarmel left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @kvoli)


pkg/kv/kvserver/asim/state/impl.go line 553 at r1 (raw file):

}

// ApplyLoad modifies the state to reflect the impact of the LoadEvent.

s/LoadEvent/LoadBatch/ here and 2 lines below.

Code quote:

LoadEvent

pkg/kv/kvserver/asim/state/impl.go line 564 at r1 (raw file):

	// Iterate in ascending order over the ranges. The load event keys are in
	// ascending order. It must be the case that at each range we visit, the
	// start key for that range is not larger than the any key of the remaining

remove the the

Code quote:

the

pkg/kv/kvserver/asim/state/impl.go line 580 at r1 (raw file):

	s.usageInfo.ApplyLoad(rng, le)

	// Note that deletes are not supported currently, also we are also assuming

remove one also (also I think I wrote that!!).

Code quote:

also we are also

This patch introduces batching for load events. Previously, load events
were generated per-key and applied individually to the simulator state
by finding the range containing that key. This patch batches load events
on the same key, then applies the load events in ascending order, over
the range tree.

This results in a speedup of 5x on a 32 store, 32k replicas, 16k QPS
cluster.

Release note: None
Copy link
Collaborator Author

@kvoli kvoli left a comment

Choose a reason for hiding this comment

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

TYFTR

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel)


pkg/kv/kvserver/asim/state/impl.go line 553 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

s/LoadEvent/LoadBatch/ here and 2 lines below.

Updated.


pkg/kv/kvserver/asim/state/impl.go line 564 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

remove the the

Updated.


pkg/kv/kvserver/asim/state/impl.go line 580 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

remove one also (also I think I wrote that!!).

Updated.

@kvoli
Copy link
Collaborator Author

kvoli commented Jun 22, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 22, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jun 22, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jun 22, 2022

Build succeeded:

@craig craig bot merged commit 6eb3bf0 into cockroachdb:master Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants