catalog: pre-compute more column slices by postamar · Pull Request #74638 · cockroachdb/cockroach

postamar · 2022-01-10T18:53:21Z

catalog: remove FullIndexColumnIDs

Calls to this function can now be replaced with recently-added
IndexFullColumns and IndexFullColumnDirections method calls on the table
descriptor.

Release note: None


catalog: add Index*ColumnDirections methods to table descriptor

This complements the previous commit which added Index*Column methods to
catalog.TableDescriptor.

Release note: None


catalog: add Index*Columns methods to table descriptor

These methods return the columns referenced in indexes as
[]catalog.Column slices.

Release note: None

cockroach-teamcity · 2022-01-10T18:53:32Z

This change is

ajwerner

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

pkg/sql/row/fetcher.go, line 469 at r3 (raw file):

	}
	for i, col := range columns {
		if col == nil || !col.Public() {

you could make this a nicer error in the case that the column is non-nil but non-public. Maybe ObjectNotInPrerequisiteState and provide the name of the column and/or the ID? I'm only pushing here because it appears this error shows up in the logic test for that funny case of trying to render a value for the conflict after a drop.

Code quote:

		if col == nil || !col.Public() {
			return nil, fmt.Errorf("column does not exist")
		}

These methods return the columns referenced in indexes as []catalog.Column slices. Release note: None

This complements the previous commit which added Index*Column methods to catalog.TableDescriptor. Release note: None

Calls to this function can now be replaced with recently-added IndexFullColumns and IndexFullColumnDirections method calls on the table descriptor. Release note: None

postamar

Thanks for the review. As always, by precomputing these column slices, we're trading off increased overhead vs. ad-hoc allocations. @nvanbenschoten The microbenchmarks for row and friends probably are worth running again after this merges. If allocations are too high, what needs to be done is not revert this but instead:

use these precomputed slices more often, instead of calls to FindColumnWithID and friends which look up columns by ID one at a time;
altogether stop maintaining slices of column IDs in row.tableInfo and friends.

A lot of this code and these data structures predate the catalog interfaces altogether and can almost certainly be streamlined. I've not done it because I don't know if it's worth doing. I estimate that doing this will take 3.6 days of work: not great, not terrible.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

pkg/sql/row/fetcher.go, line 469 at r3 (raw file):

Previously, ajwerner wrote…

you could make this a nicer error in the case that the column is non-nil but non-public. Maybe ObjectNotInPrerequisiteState and provide the name of the column and/or the ID? I'm only pushing here because it appears this error shows up in the logic test for that funny case of trying to render a value for the conflict after a drop.

Done, although I've decided not to add a pgcode. This code would end up overriding any other, which is what ended up happening in the logic test.

nvb · 2022-01-11T17:53:15Z

The microbenchmarks for row and friends probably are worth running again after this merges.

Why not run them before this merges?

go get github.com/nvanbenschoten/benchdiff

benchdiff --run='KV/Scan/SQL' ./pkg/sql/tests

postamar · 2022-01-11T18:23:30Z

It's that simple? I stupidly assumed it was going to be quite complicated. Sure, I'll do it!

nvb · 2022-01-11T18:57:50Z

Yep! Microbenchmarks are nice and easy to run, measure, and iterate on (which is why #74443 is so great). Anything system-level that needs real VMs is more involved.

postamar · 2022-01-11T22:04:21Z

I ran this microbenchmark and noticed that the perf regression was statistically significant, in some cases exceeding 2% more allocations. I pushed another commit which basically does the same pre-computations but reuses the same backing slice when possible.

We now have (on my macbook)

name                       old time/op    new time/op    delta
KV/Scan/SQL/rows=1-16         196µs ± 5%     199µs ± 3%    ~     (p=0.143 n=10+10)
KV/Scan/SQL/rows=100-16       270µs ± 3%     268µs ± 3%    ~     (p=0.497 n=10+9)
KV/Scan/SQL/rows=1000-16      857µs ± 8%     847µs ± 3%    ~     (p=0.666 n=9+9)
KV/Scan/SQL/rows=10000-16    5.05ms ± 3%    5.16ms ± 2%    ~     (p=0.101 n=10+8)
KV/Scan/SQL/rows=10-16        204µs ± 2%     209µs ± 2%  +2.11%  (p=0.022 n=10+9)

name                       old alloc/op   new alloc/op   delta
KV/Scan/SQL/rows=1000-16      222kB ± 0%     222kB ± 0%    ~     (p=0.445 n=10+8)
KV/Scan/SQL/rows=10000-16    1.14MB ± 0%    1.14MB ± 0%    ~     (p=0.258 n=9+9)
KV/Scan/SQL/rows=100-16      37.0kB ± 1%    37.2kB ± 0%  +0.71%  (p=0.000 n=9+10)
KV/Scan/SQL/rows=1-16        19.8kB ± 1%    20.1kB ± 0%  +1.44%  (p=0.000 n=10+10)
KV/Scan/SQL/rows=10-16       21.3kB ± 1%    21.7kB ± 0%  +1.47%  (p=0.000 n=9+10)

name                       old allocs/op  new allocs/op  delta
KV/Scan/SQL/rows=10000-16     50.3k ± 0%     50.3k ± 0%  +0.01%  (p=0.005 n=9+9)
KV/Scan/SQL/rows=1000-16      5.02k ± 0%     5.02k ± 0%  +0.07%  (p=0.000 n=9+9)
KV/Scan/SQL/rows=100-16         630 ± 0%       633 ± 0%  +0.43%  (p=0.000 n=6+10)
KV/Scan/SQL/rows=10-16          258 ± 0%       262 ± 0%  +1.30%  (p=0.000 n=9+10)
KV/Scan/SQL/rows=1-16           224 ± 0%       227 ± 0%  +1.38%  (p=0.000 n=10+8)

which, for what it's worth, I'm totally OK with. If this regression is a problem, then we should look into retrieving the table descriptor from the collections API instead of rebuilding it from the descriptor proto passed along in the spec.

ajwerner · 2022-01-11T22:58:58Z

I wonder if this will hurt given we end up constructing this with the table descriptor each time. This is all more fodder for using leased descriptors with distsql

This commit removes IndexCompositeColumns (which wasn't actually used and is unlikely to ever be) and generates the indexColumnCache with less memory allocations. The current scheme is causing a statistically-significant performance regression. Release note: None

postamar · 2022-01-12T01:47:40Z

Thanks for the reviews. I'll go ahead and merge this on the assumption that we'll use the leased descriptors at some point. In fact, I've already hacked something to that effect and the results are very promising.

bors r+

craig · 2022-01-12T02:30:23Z

Build succeeded:

GitHub CI (Cockroach)

postamar · 2022-01-12T03:27:42Z

c.f. #74722

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/sql/colfetcher/cfetcher.go, line 447 at r6 (raw file):

	needToDecodeDecimalKey := false
	for i, col := range fullColumns {
		if col == nil {

Saw this change when rebasing on top of master, and I'm confused by this block - when can col be nil here?

postamar · 2022-01-12T12:22:49Z

when can col be nil here?

If the ID in the index key or keys uffix column ID slice refers to a table column which doesn't exist.

postamar mentioned this pull request Jan 10, 2022

sql/catalog: restore fast-path in FullIndexColumnIDs #74341

Merged

postamar marked this pull request as ready for review January 10, 2022 20:00

postamar requested a review from a team January 10, 2022 20:00

ajwerner approved these changes Jan 11, 2022

View reviewed changes

Marius Posta added 3 commits January 11, 2022 11:58

catalog: add Index*Columns methods to table descriptor

abdbe07

These methods return the columns referenced in indexes as []catalog.Column slices. Release note: None

catalog: add Index*ColumnDirections methods to table descriptor

ba9c0f4

This complements the previous commit which added Index*Column methods to catalog.TableDescriptor. Release note: None

catalog: remove FullIndexColumnIDs

9c1f347

Calls to this function can now be replaced with recently-added IndexFullColumns and IndexFullColumnDirections method calls on the table descriptor. Release note: None

postamar force-pushed the pre-compute-more-column-slices branch from 1a55f0d to 9c1f347 Compare January 11, 2022 17:22

postamar commented Jan 11, 2022

View reviewed changes

postamar force-pushed the pre-compute-more-column-slices branch from 18e00bc to 0a39d55 Compare January 11, 2022 22:25

postamar force-pushed the pre-compute-more-column-slices branch from 0a39d55 to c3e81bc Compare January 12, 2022 00:30

craig bot merged commit 33f64b2 into cockroachdb:master Jan 12, 2022

postamar deleted the pre-compute-more-column-slices branch January 12, 2022 03:26

yuzefovich reviewed Jan 12, 2022

View reviewed changes

Conversation

postamar commented Jan 10, 2022

Uh oh!

cockroach-teamcity commented Jan 10, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

postamar left a comment

Choose a reason for hiding this comment

Uh oh!

nvb commented Jan 11, 2022

Uh oh!

postamar commented Jan 11, 2022

Uh oh!

nvb commented Jan 11, 2022

Uh oh!

postamar commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajwerner commented Jan 11, 2022

Uh oh!

postamar commented Jan 12, 2022

Uh oh!

craig bot commented Jan 12, 2022

Uh oh!

postamar commented Jan 12, 2022

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

postamar commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

postamar commented Jan 11, 2022 •

edited

Loading

postamar commented Jan 12, 2022 •

edited

Loading