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

sstable: remove iterstatsaccumulator and refactor #4272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EdwardX29
Copy link
Contributor

Removed block.IterStatsAccumulator since there is only one implementation of the interface. Refactored usages of block.IterStatsAccumulator to use CategoryStatsWithMu. Also, increased numCategoryStatsShards to be runtime.GOMAXPROCS(0).

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@EdwardX29 EdwardX29 marked this pull request as ready for review January 21, 2025 15:04
@EdwardX29 EdwardX29 requested a review from a team as a code owner January 21, 2025 15:04
Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @aadityasondhi, @EdwardX29, and @jbowens)


sstable/block/category_stats.go line 173 at r1 (raw file):

}

var numCategoryStatsShards = runtime.GOMAXPROCS(0)

how about something like max(16, runtime.GOMAXPROCS(0)).
Don't want 2 shards if someone does something silly and sets GOMAXPROCS to 2.


sstable/block/category_stats.go line 206 at r1 (raw file):

		CategoryStatsWithMu
		// Pad each shard to 64 bytes so they don't share a cache line.
		_ [64 - unsafe.Sizeof(CategoryStatsWithMu{})]byte

nit: can you lift this 64 - unsafe.Sizeof(CategoryStatsWithMu{} to a file-level const and use it here and below.

@EdwardX29 EdwardX29 force-pushed the edwardx29/remove-iterstatsaccum branch from a64d2ef to 26d48c7 Compare January 21, 2025 20:24
@EdwardX29 EdwardX29 requested a review from sumeerbhola January 21, 2025 20:50
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @aadityasondhi, @EdwardX29, @jbowens, and @sumeerbhola)


sstable/block/category_stats.go line 232 at r1 (raw file):

		v, _ = c.statsMap.LoadOrStore(category, &shardedCategoryStats{
			Category: category,
			shards: make([]struct {

[nit] define a type for this


sstable/block/category_stats.go line 242 at r1 (raw file):

	// This equation is taken from:
	// https://en.wikipedia.org/wiki/Linear_congruential_generator#Parameters_in_common_use
	shard := ((p * 25214903917) >> 32) & uint64(numCategoryStatsShards-1)

This code assumes numCategoryStatsShards is a power of two; this is not necessarily the case with GOMAXPROCS. I'd also add this requirement as a comment on numCategoryStatsShards.

@EdwardX29 EdwardX29 force-pushed the edwardx29/remove-iterstatsaccum branch from 26d48c7 to d56f31b Compare January 22, 2025 14:34
@EdwardX29 EdwardX29 requested a review from RaduBerinde January 22, 2025 17:08
@EdwardX29 EdwardX29 force-pushed the edwardx29/remove-iterstatsaccum branch from d56f31b to f040092 Compare January 22, 2025 18:41
Removed block.IterStatsAccumulator since there is only one
implementation of the interface. Refactored usages of
block.IterStatsAccumulator to use CategoryStatsWithMu.
Also, increased numCategoryStatsShards to be
max(16, the power of two greater than runtime.GOMAXPROCS(0)).
@EdwardX29 EdwardX29 force-pushed the edwardx29/remove-iterstatsaccum branch from f040092 to f9a1826 Compare January 22, 2025 18:43
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm: modulo some nits

Reviewable status: 2 of 3 files reviewed, 8 unresolved discussions (waiting on @aadityasondhi, @EdwardX29, @jbowens, and @sumeerbhola)


sstable/block/category_stats.go line 40 at r4 (raw file):

// CategoryShard is a single shard of a category's statistics.
type CategoryShard struct {

categoryShard (doesn't need to be exported)


sstable/block/category_stats.go line 182 at r4 (raw file):

}

// numCategoryStatsShards should be a power of 2 to compute shard indices

[nit] I'd clean up the phrasing as such:

numCategoryStatsShards must be a power of 2. We initialize it to GOMAXPROCS
(rounded up to the nearest power of 2) or 16, whichever is larger.

sstable/block/category_stats.go line 193 at r4 (raw file):

}()

type CategoryStatsWithMu struct {

This needs a comment. Also the name is not very useful, I think calling it CategoryStatsShard might make more sense (and name the container struct paddedCategoryStatsShard).


sstable/block/category_stats.go line 194 at r4 (raw file):

type CategoryStatsWithMu struct {
	mu sync.Mutex

Usually we do

type X struct {
  mu struct {
    sync.Mutex
    stats CategoryStats
  }
}

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.

4 participants