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.

Sign up for GitHub

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

Jump to bottom

Add different compression choice #1310

Open
wants to merge 1 commit into
base: fb-mysql-8.0.28
Choose a base branch
from
Open

Add different compression choice #1310

wants to merge 1 commit into from

Conversation

sunshine-Chun
Copy link
Contributor

Summary: when using clone to copy data in innodb instance and turn compression on. The speed is about 10 times slower than with compression off. The default compression algorithm used is zlib. Try to use other compression algorithm to tune the performance. From testing, zstd is twice faster than zlib

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

@sunshine-Chun
Copy link
Contributor Author

Need to fix test.

@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch 3 times, most recently from f2fbd59 to 30300c7 Compare May 12, 2023 22:24
@facebook-github-bot
Copy link

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

laurynas-biveinis
laurynas-biveinis reviewed May 15, 2023
View reviewed changes
plugin/clone/include/clone_client.h Outdated Show resolved Hide resolved
plugin/clone/include/clone_client.h Outdated Show resolved Hide resolved
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch from 30300c7 to e6bc11e Compare May 16, 2023 06:31
@facebook-github-bot
Copy link

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

laurynas-biveinis
laurynas-biveinis reviewed May 16, 2023
View reviewed changes
plugin/clone/include/clone.h Outdated Show resolved Hide resolved
plugin/clone/include/clone_client.h Outdated Show resolved Hide resolved
plugin/clone/src/clone_client.cc Outdated Show resolved Hide resolved
plugin/clone/src/clone_client.cc Outdated Show resolved Hide resolved
plugin/clone/src/clone_plugin.cc Outdated Show resolved Hide resolved
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch from e6bc11e to 05a9ffa Compare May 17, 2023 05:43
@facebook-github-bot
Copy link

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis
laurynas-biveinis reviewed May 17, 2023
View reviewed changes
sql/server_component/clone_protocol_service.cc Outdated Show resolved Hide resolved
plugin/clone/include/clone_client.h Outdated Show resolved Hide resolved
include/mysql/plugin_clone.h Outdated Show resolved Hide resolved
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch from 05a9ffa to 0c725aa Compare May 17, 2023 17:02
@facebook-github-bot
Copy link

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis
laurynas-biveinis reviewed May 18, 2023
View reviewed changes
Copy link
Contributor

@laurynas-biveinis laurynas-biveinis left a comment

Choose a reason for hiding this comment

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

LGTM

@sunshine-Chun
Copy link
Contributor Author

sunshine-Chun commented May 18, 2023 •
edited
Loading

Looks like we have memory leakage from valgrind test. Need to debug a bit. Will update here once find anything.

  at 0xA5A6795: malloc (vg_replace_malloc.c:381)
   by 0x90917AD: ZSTD_customMalloc (allocations.h:30)
   by 0x90917AD: ZSTD_createDCtx_internal (zstd_decompress.c:296)
   by 0x90917AD: ZSTD_createDCtx (zstd_decompress.c:312)
by 0x7700BEA: zstd_uncompress(mysql_zstd_compress_context*, unsigned char*, unsigned long, unsigned long*) [clone .__uniq.287700427806220246369010965997275247218] (my_compress.cc:232)
   by 0x77009DE: my_uncompress(mysql_compress_context*, unsigned char*, unsigned long, unsigned long*) (my_compress.cc:699)
   by 0x64D4473: net_read_compressed_packet(NET*, unsigned long&) [clone .__uniq.40191296941203724010080517394248553221] (net_serv.cc:2259)
   by 0x64D4364: my_net_read(NET*) (net_serv.cc:2297)
   by 0x85CBC4D: mysql_clone_get_response(THD*, MYSQL*, bool, unsigned int, unsigned char**, unsigned long*, unsigned long*) (clone_protocol_service.cc:545)

@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch from 0c725aa to 13690e4 Compare May 19, 2023 22:52
@facebook-github-bot
Copy link

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@sunshine-Chun
Copy link
Contributor Author

Memory leakage fixed.

@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch from 13690e4 to a394a4f Compare May 19, 2023 22:57
@facebook-github-bot
Copy link

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

laurynas-biveinis
laurynas-biveinis reviewed May 22, 2023
View reviewed changes
Copy link
Contributor

@laurynas-biveinis laurynas-biveinis left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment formatting issue fixed

sql/server_component/clone_protocol_service.cc Outdated
@@ -539,6 +545,13 @@ DEFINE_METHOD(int, mysql_clone_get_response,
*net_length = 0;
*length = my_net_read(net);

/** The ZSTD object has been updated to include the newly created decompressor
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a Doxygen comment:

Suggested change
/** The ZSTD object has been updated to include the newly created decompressor
/* The ZSTD object has been updated to include the newly created decompressor

sql/server_component/clone_protocol_service.cc Outdated
@@ -539,6 +545,13 @@ DEFINE_METHOD(int, mysql_clone_get_response,
*net_length = 0;
*length = my_net_read(net);

/** The ZSTD object has been updated to include the newly created decompressor
* context. Include the change in the net extension so that the resource can
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the next line: the comment style with leading stars is not used in MySQL:

Suggested change
* context. Include the change in the net extension so that the resource can
context. Include the change in the net extension so that the resource can

@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch from a394a4f to abc04b7 Compare May 30, 2023 16:18
@facebook-github-bot
Copy link

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.

@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch from abc04b7 to 6d5db1f Compare May 30, 2023 17:41
@facebook-github-bot
Copy link

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@sunshine-Chun
Add different compression choice
b79ea9f 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.28-cp-compression branch from 6d5db1f to b79ea9f Compare June 2, 2023 16:48
@facebook-github-bot
Copy link

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurynas-biveinis laurynas-biveinis laurynas-biveinis left review comments

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sunshine-Chun @facebook-github-bot @laurynas-biveinis