exec: Add memory estimation and monitoring for streaming operators. #38796

rohany · 2019-07-10T18:59:56Z

We want to error out early of our vectorized execution if there is not enough memory available to run the query, especially if we can tell upfront that this is the case. Some streaming operators always use a static amount of memory, so we can monitor this memory during construction of the vectorized plan. Due to difficulties with traversing the vectorized flow once it is constructed, we monitor memory during construction of each operator, and have streaming operators estimate how much memory they will use during construction. This PR adds memory estimation to the following operators:

CountOp
Aggregate operators
TopK sorter
Columnarizer
Coalescer
OrderedSynchronizer
Projection operators

Release note: None

cockroach-teamcity · 2019-07-10T19:00:07Z

This change is

rohany · 2019-07-10T19:00:27Z

This is pretty big PR. Reviews + discussion are very much appreciated. Let me know if I missed any operators!

asubiotto

I like the general approach, although I'm wondering if it would be cleaner to either 1) just return the number of bytes that an operator will use or 2) Have the operators you care about implement an interface that will return the number of bytes used given some types and in either case increment the account in newColOperator after creating an operator and then again after creating any post processing operators. I like the second approach better and it'll be nice to not have to care about the mon package, modify tests that don't care about memory, or modify constructors. We'll have to do something similar for buffering operators and since they will return the same amount of memory regardless of the types.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @rohany, and @solongordon)

pkg/sql/distsqlrun/column_exec_setup.go, line 554 at r1 (raw file):

func planSelectionOperators(
	ctx context.Context,
	tctx *tree.EvalContext,

nit: s/tctx/evalCtx

pkg/sql/distsqlrun/columnar_utils_test.go, line 76 at r1 (raw file):

	columnarizers := make([]exec.Operator, len(inputs))
	for i, input := range inputsColOp {
		c, err := newColumnarizer(ctx, flowCtx, int32(i)+1, input, nil)

nit: add block comments to nil arguments

pkg/sql/distsqlrun/flow_vectorize_space_test.go, line 72 at r1 (raw file):

	}{
		{
			desc: "Test construct sorttopk",

nit: use camel case and no spaces, there's also no need to be overly descriptive (e.g. TopK is enough in this case, on failure this will be printed as TestVectorizeSpaceError/TopK which is informative enough). It's also nice to use short names to minimize typing mistakes for when you need to rerun a subset of tests

pkg/sql/exec/mem_estimation.go, line 32 at r1 (raw file):

			// much space each byte array takes up. Use some default value as a
			// heuristic right now.
			acc += 100

I would put a big warning at the top that this function only really works for fixed-width types and maybe mention that there will be a transition to specifying batch sizes in terms of bytes, which will remove the need for any estimation.

pkg/sql/exec/mem_estimation.go, line 36 at r1 (raw file):

			acc++
		case types.Int16:
			acc += 2

nit: You could improve readability by extracting constants and using them here:

const (
	sizeOfInt8    = int(unsafe.Sizeof(int8(0)))
	sizeOfInt16   = int(unsafe.Sizeof(int16(0)))
	sizeOfInt32   = int(unsafe.Sizeof(int32(0)))
	sizeOfInt64   = int(unsafe.Sizeof(int64(0)))
	sizeOfFloat32 = int(unsafe.Sizeof(float32(0)))
	sizeOfFloat64 = int(unsafe.Sizeof(float64(0)))
)

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @solongordon)

pkg/sql/exec/mem_estimation.go, line 32 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I would put a big warning at the top that this function only really works for fixed-width types and maybe mention that there will be a transition to specifying batch sizes in terms of bytes, which will remove the need for any estimation.

Done.

pkg/sql/exec/mem_estimation.go, line 36 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: You could improve readability by extracting constants and using them here:

const (
	sizeOfInt8    = int(unsafe.Sizeof(int8(0)))
	sizeOfInt16   = int(unsafe.Sizeof(int16(0)))
	sizeOfInt32   = int(unsafe.Sizeof(int32(0)))
	sizeOfInt64   = int(unsafe.Sizeof(int64(0)))
	sizeOfFloat32 = int(unsafe.Sizeof(float32(0)))
	sizeOfFloat64 = int(unsafe.Sizeof(float64(0)))
)

Ok, I wasn't sure if it was ok to use the unsafe package or not.

rohany · 2019-07-15T21:07:16Z

RFAL -- followed alfonso's suggestion, and the code seems cleaner than before.

asubiotto

Nice, I think it does look cleaner although this is making me think that fully integrating #38394 into the allocation flow will probably be beneficial for streaming operators as well to both verify that memory declared is not too far off from actual memory requested (this would be great for logic tests)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @rohany, and @solongordon)

pkg/sql/distsqlrun/column_exec_setup.go, line 102 at r2 (raw file):

	spec *distsqlpb.ProcessorSpec,
	inputs []exec.Operator,
) (exec.Operator, []types.T, int, error) {

Seems like we might as well make these named return variables at this point

pkg/sql/distsqlrun/column_exec_setup.go, line 479 at r2 (raw file):

	// After constructing the base operator, calculate the memory usage
	// of the operator.
	if sMemOp, ok := op.(exec.StaticMemoryOperator); ok {

When we talked about this, did we mention that we sometimes created multiple operators in the above switch? Skimming it, I see it in the windower case, which is not something we care about right now but want to be sure we're not missing a case in the static operator creation.

pkg/sql/distsqlrun/column_exec_setup.go, line 495 at r2 (raw file):

	if !post.Filter.Empty() {
		var helper exprHelper
		var memUsed int

nit: s/memUsed/selectionMem or something similar. Also, another way to declare variables on multiple lines is:

var (
    helper exprHelper
    memUsed int
)

Up to you

pkg/sql/distsqlrun/column_exec_setup.go, line 919 at r2 (raw file):

				return nil, nil, memUsed, err
			}
			inbox, err := colrpc.NewInbox(conv.FromColumnTypes(input.ColumnTypes))

I think we need to account for the static memory used by the inbox here as well, otherwise we won't be counting remote data.

pkg/sql/distsqlrun/column_exec_setup.go, line 1064 at r2 (raw file):

			}
			if err = acc.Grow(ctx, int64(memUsed)); err != nil {
				return errors.Wrapf(err, "Not enough memory to setup vectorized plan.")

nit: error message aren't capitalized or have punctuation by convention (https://github.com/golang/go/wiki/CodeReviewComments#error-strings)

pkg/sql/distsqlrun/flow_vectorize_space_test.go, line 28 at r2 (raw file):

)

func TestVectorizeSpaceError(t *testing.T) {

I wonder if there's a way to do some black box testing of this addition (maybe not now). This will probably be easier with the addition of @solongordon's BatchAllocator but if we had some way to globally track the batches actually allocated, we could probably add a testing knob to logic tests similar to metadata verification that would then verify that memory usage reported to the monitor is around what was requested from the BatchAllocator.

pkg/sql/distsqlrun/flow_vectorize_space_test.go, line 108 at r2 (raw file):

		},
		{
			desc: "aggergation",

s/aggergation/aggregation

pkg/sql/distsqlrun/flow_vectorize_space_test.go, line 127 at r2 (raw file):

						ctx, "Unlimited Monitor", mon.MemoryResource, nil, nil, math.MaxInt64, st)
				} else {
					memMon = mon.MakeMonitorWithLimit(

It's a bit subtle, but hard-limit monitors like this one are only used in processors that fall back to disk. To mirror the monitor that is used in setupVectorized you have to do something like:

memMon := mon.MakeMonitor(...)
if succ {
    memMon.Start(..., mon.MakeStandaloneBudget(math.MaxInt64))
} else {
    memMon.Start(..., mon.MakeStandaloneBudget(1))
}
defer memMon.Stop()

pkg/sql/distsqlrun/flow_vectorize_space_test.go, line 134 at r2 (raw file):

				err = acc.Grow(ctx, int64(memUsed))
				if succ && err != nil {
					t.Fatal("Expected success, found: ", err)

nit: I think this will print double spaces

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @rohany, and @solongordon)

pkg/sql/distsqlrun/column_exec_setup.go, line 102 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Seems like we might as well make these named return variables at this point

Done.

pkg/sql/distsqlrun/column_exec_setup.go, line 479 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

When we talked about this, did we mention that we sometimes created multiple operators in the above switch? Skimming it, I see it in the windower case, which is not something we care about right now but want to be sure we're not missing a case in the static operator creation.

I went through the cases and made sure to increment the memory usage when we layer operators, or a StaticMemoryOperator gets layered over.

pkg/sql/distsqlrun/column_exec_setup.go, line 919 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think we need to account for the static memory used by the inbox here as well, otherwise we won't be counting remote data.

Done.

pkg/sql/distsqlrun/flow_vectorize_space_test.go, line 108 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

s/aggergation/aggregation

Done.

pkg/sql/distsqlrun/flow_vectorize_space_test.go, line 127 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

It's a bit subtle, but hard-limit monitors like this one are only used in processors that fall back to disk. To mirror the monitor that is used in setupVectorized you have to do something like:
memMon := mon.MakeMonitor(...)
if succ {
    memMon.Start(..., mon.MakeStandaloneBudget(math.MaxInt64))
} else {
    memMon.Start(..., mon.MakeStandaloneBudget(1))
}
defer memMon.Stop()

Done.

asubiotto

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @rohany, and @solongordon)

pkg/sql/distsqlrun/column_exec_setup.go, line 1108 at r3 (raw file):

		op, outputTypes, memUsage, err := newColOperator(ctx, &f.FlowCtx, pspec, inputs)
		if err != nil {
			return errors.Wrapf(err, "Unable to vectorize execution plan.")

nit: same thing here and below about the error messages

We want to error out early of our vectorized execution if there is not enough memory available to run the query, especially if we can tell upfront that this is the case. Some streaming operators always use a static amount of memory, so we can monitor this memory during construction of the vectorized plan. Due to difficulties with traversing the vectorized flow once is it constructed, we monitor memory during construction of each operator, and have streaming operators estimate how much memory they will use during construction. This PR adds memory estimation to the following operators: * CountOp * Aggregate operators * TopK sorter * Columnarizer * Coalescer * OrderedSynchronizer * Projection operators Release note: None

rohany · 2019-07-17T16:50:56Z

bors r=asubiotto

38796: exec: Add memory estimation and monitoring for streaming operators. r=asubiotto a=rohany We want to error out early of our vectorized execution if there is not enough memory available to run the query, especially if we can tell upfront that this is the case. Some streaming operators always use a static amount of memory, so we can monitor this memory during construction of the vectorized plan. Due to difficulties with traversing the vectorized flow once it is constructed, we monitor memory during construction of each operator, and have streaming operators estimate how much memory they will use during construction. This PR adds memory estimation to the following operators: * CountOp * Aggregate operators * TopK sorter * Columnarizer * Coalescer * OrderedSynchronizer * Projection operators Release note: None Co-authored-by: Rohan Yadav <[email protected]>

craig · 2019-07-17T17:18:40Z

Build succeeded

GitHub CI (Cockroach)

asubiotto · 2019-07-17T19:17:23Z

Something I just realized: I think we might need to close the vectorized bound account on setup error as well, otherwise we'll never clear the memory when we fail to set up a vectorized flow in some cases. Failures on remote nodes won't ever call Cleanup on an error, the only case I can think of that might be fine is when falling back to distsql on gateway nodes, since we use the same flow struct, but that still means that we'll have extra accounted for memory during the execution of that flow.

asubiotto · 2019-07-17T19:44:37Z

I'm touching this code now so will fix.

rohany · 2019-07-17T21:10:32Z

That makes sense -- good catch.

rohany requested review from jordanlewis, solongordon, asubiotto and a team July 10, 2019 18:59

rohany mentioned this pull request Jul 11, 2019

exec: Add support for vectorized engine to use builtin functions. #38826

Merged

rohany force-pushed the vec-mem-constructor branch from 92f6093 to 9ee8613 Compare July 11, 2019 21:35

asubiotto reviewed Jul 15, 2019

View reviewed changes

rohany force-pushed the vec-mem-constructor branch from 9ee8613 to 9c12409 Compare July 15, 2019 21:06

rohany commented Jul 15, 2019

View reviewed changes

solongordon requested a review from a team July 16, 2019 14:14

asubiotto suggested changes Jul 16, 2019

View reviewed changes

rohany force-pushed the vec-mem-constructor branch from 9c12409 to 0665485 Compare July 16, 2019 16:53

rohany commented Jul 16, 2019

View reviewed changes

rohany force-pushed the vec-mem-constructor branch 3 times, most recently from 918789f to 2f1d295 Compare July 16, 2019 18:36

asubiotto approved these changes Jul 17, 2019

View reviewed changes

rohany force-pushed the vec-mem-constructor branch from 2f1d295 to b9d62ca Compare July 17, 2019 15:51

craig bot merged commit b9d62ca into cockroachdb:master Jul 17, 2019

rohany mentioned this pull request Jul 17, 2019

exec: memory estimation for streaming operators #38658

Closed

knz mentioned this pull request Nov 10, 2019

User-facing changes in 19.2 that were not picked up in release notes cockroachdb/docs#5819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exec: Add memory estimation and monitoring for streaming operators. #38796

exec: Add memory estimation and monitoring for streaming operators. #38796

rohany commented Jul 10, 2019 •

edited by yuzefovich

Loading

cockroach-teamcity commented Jul 10, 2019

rohany commented Jul 10, 2019

asubiotto left a comment

rohany left a comment

rohany commented Jul 15, 2019

asubiotto left a comment

rohany left a comment

asubiotto left a comment

rohany commented Jul 17, 2019

craig bot commented Jul 17, 2019

asubiotto commented Jul 17, 2019

asubiotto commented Jul 17, 2019

rohany commented Jul 17, 2019

exec: Add memory estimation and monitoring for streaming operators. #38796

exec: Add memory estimation and monitoring for streaming operators. #38796

Conversation

rohany commented Jul 10, 2019 • edited by yuzefovich Loading

cockroach-teamcity commented Jul 10, 2019

rohany commented Jul 10, 2019

asubiotto left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

rohany commented Jul 15, 2019

asubiotto left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

asubiotto left a comment

Choose a reason for hiding this comment

rohany commented Jul 17, 2019

craig bot commented Jul 17, 2019

Build succeeded

asubiotto commented Jul 17, 2019

asubiotto commented Jul 17, 2019

rohany commented Jul 17, 2019

rohany commented Jul 10, 2019 •

edited by yuzefovich

Loading