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

util/mon: reduce monitor-related allocations #143305

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Mar 21, 2025

util/mon: rename fields of MonitorName

Release note: None

util/uuid: add comment to uuid.Short

Release note: None

util/mon: add fields to MonitorName

The processorID, suffix, and i fields will be used in future
commits to reduce allocations related to MonitorNames.

Release note: None

util/mon: compile-time checks for size of BytesMonitor and BoundAccount

Release note: None

sql: use mon.MonitorName in more places

Using structured mon.MonitorName objects reduces allocations of
strings in some cases. This commit doesn't replace all usages of string
monitor names - there are myriad such usages. We can iteratively chip
away at them in future commits.

Release note: None

util/mon: reduce size of MonitorName

The size of MonitorName has been reduced by replacing the string
suffix with a suffix enum.

Release note: None

util/mon: further reduce the size of MonitorName

The size of MonitorName has been reduced by using the same 4 bytes to
store either the int32 processor ID or the uuid.Short, and by
reducing the optional integer suffix from an int32 to uint16, which
should be sufficient in all cases.

Release note: None

util/mon: rename MonitorName to Name

Release note: None

util/mon: remove deprecated NewMonitorWithStringName

All usages of the deprecated NewMonitorWithStringName function have been
replaced with NewMonitor, and the former has been removed.

Release note: None

util/mon: remove deprecated NewMonitorInheritWithLimitAndStringName

All usages of the deprecated NewMonitorInheritWithLimitAndStringName
function have been replaced with NewMonitorInheritWithLimit, and the
former has been removed.

Release note: None

colexec: batch allocations of BoundAccount

Allocations of BoundAccounts in createUnlimitedMemAccountsLocked are
now batched into a single allocation.

Release note: None

sql/rowflow: refactor monitor name

The monitor names created in (*rowBasedFlow).setupRouter now use
(*mon.Name).WithID to add the stream ID to the name instead of
allocating a new string.

Epic: None
Release note: None

@mgartner mgartner added the o-perf-efficiency Related to performance efficiency label Mar 21, 2025
@mgartner mgartner requested review from a team as code owners March 21, 2025 20:44
@mgartner mgartner requested review from yuzefovich, jbowens, kev-cao, asg0451 and xinhaoz and removed request for a team March 21, 2025 20:44
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner
Copy link
Collaborator Author

mgartner commented Mar 21, 2025

It's a very minor reduction in allocations. But it is nice to have structured names instead of strings.

name                                              old allocs/op  new allocs/op  delta
FlowSetup/vectorize=true/distribute=true               162 ± 1%       160 ± 0%  -1.31%  (p=0.000 n=16+16)
FlowSetup/vectorize=true/distribute=false              138 ± 4%       136 ± 4%  -1.32%  (p=0.000 n=17+17)
FlowSetup/vectorize=false/distribute=true              160 ± 1%       160 ± 0%    ~     (p=0.455 n=17+16)
FlowSetup/vectorize=false/distribute=false             135 ± 0%       135 ± 0%    ~     (all equal)

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Nice cleanup! :lgtm:

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 33 of 33 files at r5, 8 of 8 files at r6, 3 of 3 files at r7, 77 of 77 files at r8, 17 of 17 files at r9, 14 of 14 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @asg0451, @jbowens, @kev-cao, and @xinhaoz)


pkg/util/mon/bytes_usage.go line 281 at r8 (raw file):

}

// Name is used to identify monitors in logging messages. It consists of:

nit: let's also mention crdb_internal.node_memory_monitors virtual table where the monitor name shows up.


pkg/sql/rowflow/row_based_flow.go line 464 at r9 (raw file):

	diskMonitors := make([]*mon.BytesMonitor, len(spec.Streams))
	for i := range spec.Streams {
		// TODO(mgartner): Is a StreamID ever greater than math.MaxInt32? If

I don't think it can ever exceed max int32 in practice. StreamIDs are indexes into PhysicalInfrastructure.Streams slice, which contains all pair-wise "links" between two processors within the same plan, so I'd expect that we'd OOM-crash if stream ID reaches 1 million, way before max int 32.

The `processorID`, `suffix`, and `i` fields will be used in future
commits to reduce allocations related to MonitorNames.

Release note: None
Using structured `mon.MonitorName` objects reduces allocations of
strings in some cases. This commit doesn't replace all usages of string
monitor names - there are myriad such usages. We can iteratively chip
away at them in future commits.

Release note: None
The size of `MonitorName` has been reduced by replacing the string
suffix with a suffix enum.

Release note: None
The size of `MonitorName` has been reduced by using the same 4 bytes to
store either the `int32` processor ID or the `uuid.Short`, and by
reducing the optional integer suffix from an `int32` to `uint16`, which
should be sufficient in all cases.

Release note: None
All usages of the deprecated NewMonitorWithStringName function have been
replaced with NewMonitor, and the former has been removed.

Release note: None
All usages of the deprecated NewMonitorInheritWithLimitAndStringName
function have been replaced with NewMonitorInheritWithLimit, and the
former has been removed.

Release note: None
Allocations of `BoundAccount`s in `createUnlimitedMemAccountsLocked` are
now batched into a single allocation.

Release note: None
The monitor names created in `(*rowBasedFlow).setupRouter` now use
`(*mon.Name).WithID` to add the stream ID to the name instead of
allocating a new string.

Release note: None
@mgartner mgartner force-pushed the refactor-monitor-name branch from 92c6177 to 7d4c6ac Compare March 25, 2025 21:21
Copy link
Collaborator Author

@mgartner mgartner 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: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asg0451, @jbowens, @kev-cao, @xinhaoz, and @yuzefovich)


pkg/sql/rowflow/row_based_flow.go line 464 at r9 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I don't think it can ever exceed max int32 in practice. StreamIDs are indexes into PhysicalInfrastructure.Streams slice, which contains all pair-wise "links" between two processors within the same plan, so I'd expect that we'd OOM-crash if stream ID reaches 1 million, way before max int 32.

Makes sense, I'll add another commit to address this TODO then. Thanks!


pkg/util/mon/bytes_usage.go line 281 at r8 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: let's also mention crdb_internal.node_memory_monitors virtual table where the monitor name shows up.

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
o-perf-efficiency Related to performance efficiency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants