Skip to content

[go/mysql] use tiered pool for buffers to avoid false hits#4183

Merged
sougou merged 1 commit intovitessio:masterfrom
LK4D4:tiered_pool
Sep 6, 2018
Merged

[go/mysql] use tiered pool for buffers to avoid false hits#4183
sougou merged 1 commit intovitessio:masterfrom
LK4D4:tiered_pool

Conversation

@LK4D4
Copy link
Copy Markdown
Contributor

@LK4D4 LK4D4 commented Sep 5, 2018

Now when we get smaller than we want buffer from pool, we just put it
back and allocate a new one. This might be quite inefficient.
So, instead use multiple pools which always return buffers >= request
size.
Benchmark results:

benchmark                            old ns/op     new ns/op     delta
BenchmarkParallelShortQueries-8      3347          3327          -0.60%
BenchmarkParallelMediumQueries-8     6535          5789          -11.42%
BenchmarkParallelRandomQueries-8     6001539       5453566       -9.13%

benchmark                            old MB/s     new MB/s     speedup
BenchmarkParallelShortQueries-8      3.29         3.31         1.01x
BenchmarkParallelMediumQueries-8     2507.72      2830.78      1.13x

benchmark                            old allocs     new allocs     delta
BenchmarkParallelShortQueries-8      23             23             +0.00%
BenchmarkParallelMediumQueries-8     9              8              -11.11%
BenchmarkParallelRandomQueries-8     15             14             -6.67%

benchmark                            old bytes     new bytes     delta
BenchmarkParallelShortQueries-8      720           720           +0.00%
BenchmarkParallelMediumQueries-8     27243         20865         -23.41%
BenchmarkParallelRandomQueries-8     36496086      29452887      -19.30%

This is new package and internal to vitess. So, we can make interface less flexible maybe. Like allow only powers of two and say that multiplier is always 2.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Sep 5, 2018

Let's use a human-readable simplification:
minSize: 1
maxSize: 16
multiipler: 2

This will yield the following buckets: 1, 2, 4, 8, 16.

Now, if we perform log operations, with multiplier as base(2), it will yield the index of the pool.
The respective logs for the above will be: 0, 1, 2, 3, 4.

Let's say requested length was: 5. Log(5) = 2.3, which rounds up to 3.
Log(9) -> 3.1, rounds up to 4.

This approach will allow us to have a larger number of pools. We should have pools of size up to 16M.
Hopefully, log functions aren't too expensive :).

For minSize other than 1, we have to first divide by minSize before taking the log.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Sep 5, 2018

As for returning the last index, the idea is to put everything above the max length into that bucket and take a gamble. The next one that pulls from the pool will have to check the length.

But I think that's not worth it. So, it's better to just allocate from heap.

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Sep 5, 2018

@sougou is it okay to fix multiplier as 2? Because there is no arbitrary base logarithm in go it seems.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Sep 5, 2018

Yeah. I was surprised about that. But 2 is a good multiplier to hard-code.
Let's also run this benchmark to see how fast it goes: https://golang.org/src/math/all_test.go#L3404 :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

15000?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Now when we get smaller than we want buffer from pool, we just put it
back and allocate a new one. This might be quite inefficient.
So, instead use multiple pools which always return buffers >= request
size.

Signed-off-by: Alexander Morozov <lk4d4math@gmail.com>
@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Sep 5, 2018

@sougou @danieltahara PTAL
This should be really well-tested now :)

Copy link
Copy Markdown

@danieltahara danieltahara left a comment

Choose a reason for hiding this comment

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

Looks good! One note about deprecating the huge allocation path (length >= MaxPacketSize), since it's handled gracefully here.

if length < MaxPacketSize {
c.currentEphemeralPolicy = ephemeralReadSingleBuffer
c.currentEphemeralReadBuffer = getBuf(length)
c.currentEphemeralReadBuffer = bufPool.Get(length)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we should be able to remove the huge allocation path as well, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is one path with direct which makes something different IIRC. I will take a closer look.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yeah i think we can defer to c.buffer diff

@danieltahara
Copy link
Copy Markdown

@sougou jumping back over here -- since this doesn't get rid of c.buffer yet, is this good to go?

@sougou sougou merged commit 5b4500c into vitessio:master Sep 6, 2018
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