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

kv/tscache: dynamically size intervalSkl pages #49422

Merged
merged 2 commits into from
May 23, 2020

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented May 21, 2020

Addresses a suggestion from Peter: #48058 (comment).

This change updates intervalSkl to dynamically grow the size
of its pages as pages are rotated. This allows the structure to
start off small (128 KB per page) and grow logarithmically to a
maximum size (32 MB per page) as it is used. The pages start
small to limit the memory footprint of the data structure for
short-lived tests but will settle upon the maximum page size
under steady-state on a long-lived process.

This does not appear to have an impact on benchmarks:

➜ benchdiff --run='BenchmarkJoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384' ./pkg/sql/rowexec
checking out 'f575fa8'
building benchmark binaries for 'f575fa8' 1/1 -
checking out '3d46054'
building benchmark binaries for '3d46054' 1/1 /

  pkg=1/1 iter=10/10 cockroachdb/cockroach/pkg/sql/rowexec |

name                                                                        old time/op    new time/op    delta
JoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384-16     1.34s ± 2%     1.34s ± 4%    ~     (p=1.000 n=9+10)

name                                                                        old speed      new speed      delta
JoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384-16  3.23MB/s ± 2%  3.23MB/s ± 3%    ~     (p=0.953 n=9+10)

name                                                                        old alloc/op   new alloc/op   delta
JoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384-16    72.1MB ± 0%    73.9MB ± 0%  +2.44%  (p=0.000 n=10+10)

name                                                                        old allocs/op  new allocs/op  delta
JoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384-16      556k ± 0%      556k ± 0%    ~     (p=0.781 n=10+10)

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @petermattis)


pkg/kv/kvserver/tscache/interval_skl.go, line 164 at r1 (raw file):

	// The size of the last allocated page in the data structure, in bytes. When
	// a page fills, a new page will be allocate, the pages will be rotated, and
	// older entries will be discarded. Page sizes grow logarithmically as pages

Isn't this normally described as exponential growth?


pkg/kv/kvserver/tscache/interval_skl.go, line 172 at r1 (raw file):

	// intervalSkl needs to grow larger to enforce a minimum retention policy.
	pageSize      uint32
	pageSizeFixed bool

Might want to mention this is only used by tests.

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/tscacheSize branch from a753bf2 to b1b556f Compare May 22, 2020 23:05
Addresses a suggestion from Peter: cockroachdb#48058 (comment).

This change updates `intervalSkl` to dynamically grow the size
of its pages as pages are rotated. This allows the structure to
start off small (128 KB per page) and grow exponentially to a
maximum size (32 MB per page) as it is used. The pages start
small to limit the memory footprint of the data structure for
short-lived tests but will settle upon the maximum page size
under steady-state on a long-lived process.

This does not appear to have an impact on benchmarks:
```
➜ benchdiff --run='BenchmarkJoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384' ./pkg/sql/rowexec
checking out 'f575fa8'
building benchmark binaries for 'f575fa8' 1/1 -
checking out '3d46054'
building benchmark binaries for '3d46054' 1/1 /

  pkg=1/1 iter=10/10 cockroachdb/cockroach/pkg/sql/rowexec |

name                                                                        old time/op    new time/op    delta
JoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384-16     1.34s ± 2%     1.34s ± 4%    ~     (p=1.000 n=9+10)

name                                                                        old speed      new speed      delta
JoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384-16  3.23MB/s ± 2%  3.23MB/s ± 3%    ~     (p=0.953 n=9+10)

name                                                                        old alloc/op   new alloc/op   delta
JoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384-16    72.1MB ± 0%    73.9MB ± 0%  +2.44%  (p=0.000 n=10+10)

name                                                                        old allocs/op  new allocs/op  delta
JoinReader/reqOrdering=false/matchratio=onetothirtytwo/lookuprows=16384-16      556k ± 0%      556k ± 0%    ~     (p=0.781 n=10+10)
```
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/tscacheSize branch from b1b556f to 1886885 Compare May 22, 2020 23:06
Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

TFTR!

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


pkg/kv/kvserver/tscache/interval_skl.go, line 164 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Isn't this normally described as exponential growth?

Heh, yes. This is systems programming, not machine learning. We don't use logarithms around here.


pkg/kv/kvserver/tscache/interval_skl.go, line 172 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Might want to mention this is only used by tests.

Done.

@nvanbenschoten
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented May 23, 2020

Build failed

@nvanbenschoten
Copy link
Member Author

testrace failed with the following error: Failed to perform checkout on agent: '/usr/bin/git -c credential.helper= checkout -q -f staging' command failed. Looks like a flake, maybe due to the issues Github has been seeing.

bors r+

@craig
Copy link
Contributor

craig bot commented May 23, 2020

Build succeeded

@craig craig bot merged commit 8671fd6 into cockroachdb:master May 23, 2020
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/tscacheSize branch May 27, 2020 19:55
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.

3 participants