Skip to content

[go/mysql] use sync.Pool for write buffers#4162

Merged
sougou merged 1 commit intovitessio:masterfrom
LK4D4:pool_write_buffers
Sep 5, 2018
Merged

[go/mysql] use sync.Pool for write buffers#4162
sougou merged 1 commit intovitessio:masterfrom
LK4D4:pool_write_buffers

Conversation

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Aug 28, 2018

We already use such strategy for read buffers, so it makes sense to
do the same for writes.
write buffer makes a lot of one-times buffers which never reused and increase pressure on GC:

Total: 1818.29GB
ROUTINE ======================== vitess.io/vitess/go/mysql.(*Conn).startEphemeralPacket in go/src/vitess.io/vitess/go/mysql/conn.go
  458.13GB   458.13GB (flat, cum) 25.20% of Total
         .          .    556:   }
         .          .    557:
         .          .    558:   // Slower path: we can use a single buffer for both the header and the data, but it has to be allocated.
         .          .    559:   if length < MaxPacketSize {
         .          .    560:           c.currentEphemeralPolicy = ephemeralWriteSingleBuffer
  458.13GB   458.13GB    561:           c.currentEphemeralPacket = make([]byte, length+4)
         .          .    562:           c.currentEphemeralPacket[0] = byte(length)
         .          .    563:           c.currentEphemeralPacket[1] = byte(length >> 8)
         .          .    564:           c.currentEphemeralPacket[2] = byte(length >> 16)

danieltahara
danieltahara reviewed Aug 28, 2018
View reviewed changes
go/mysql/conn.go Outdated
Copy link

@danieltahara danieltahara Aug 28, 2018

Choose a reason for hiding this comment

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

Can you add a comment here delineating between this and currentEphemeralBuffer? Might even be worthwhile to rename to currentEphemeralWriteBuffer, currentEphemeralReadBuffer.

@danieltahara danieltahara requested a review from demmer August 28, 2018 19:36
@danieltahara
Copy link

danieltahara commented Aug 28, 2018

Related #4138 #3718

@danieltahara
Copy link

danieltahara commented Aug 29, 2018

After:

Total: 2361.91GB
ROUTINE ======================== vitess.io/vitess/go/mysql.(*Conn).startEphemeralPacket in go/src/vitess.io/vitess/go/mysql/conn.go
         0    33.14GB (flat, cum)  1.40% of Total
         .          .    563:
         .          .    564:   // Slower path: we can use a single buffer for both the header and the data, but it has to be allocated.
         .          .    565:   if length < MaxPacketSize {
         .          .    566:           c.currentEphemeralPolicy = ephemeralWriteSingleBuffer
         .          .    567:
         .    33.14GB    568:           c.currentEphemeralWriteBuffer = getBuf(length + 4)
         .          .    569:           (*c.currentEphemeralWriteBuffer)[0] = byte(length)

sougou
sougou reviewed Sep 1, 2018
View reviewed changes
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Couple of minor comments/questions. I'll give more high level comments on the associated issue.

go/mysql/conn.go Outdated
Copy link
Contributor

@sougou sougou Sep 1, 2018

Choose a reason for hiding this comment

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

This looks like a no-op. Is this intentional?

@LK4D4
Copy link
Contributor Author

LK4D4 commented Sep 4, 2018

@sougou can you please help me to understand travis failure?

@LK4D4
[go/mysql] use sync.Pool for write buffers
22d47a7 
We already use such strategy for read buffers, so it makes sense to
do the same for writes.

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

LK4D4 commented Sep 4, 2018

@sougou looks like it was the flake :/
PTAL

@sougou sougou merged commit e4d51cf into vitessio:master Sep 5, 2018
@zmagg zmagg mentioned this pull request Sep 7, 2018
Merged
@LK4D4 LK4D4 deleted the pool_write_buffers branch March 26, 2019 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alainjobart alainjobart Awaiting requested review from alainjobart

@demmer demmer Awaiting requested review from demmer

2 more reviewers

@danieltahara danieltahara danieltahara left review comments

@sougou sougou sougou approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LK4D4 @danieltahara @sougou