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

perf(tools): yield when gzip or gunzip #13338

Merged
merged 3 commits into from
Jul 23, 2024
Merged

perf(tools): yield when gzip or gunzip #13338

merged 3 commits into from
Jul 23, 2024

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Jul 5, 2024 •
edited
Loading

Summary

KAG-4878

Still in discussion, but I checked the source of ffi-zlib and believe it can yield safely.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #[issue number]
@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jul 5, 2024
@chronolaw chronolaw changed the title feat(tools): yield when gzip or ungzip Jul 8, 2024
@chronolaw chronolaw marked this pull request as ready for review July 8, 2024 02:00
zhongweiy
zhongweiy reviewed Jul 8, 2024
View reviewed changes
kong/tools/gzip.lua
count = 0
yield()
end

local data = input_buffer:get(size)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking whether input_buffer:get() itself can yield if the input_buffer is set to a socket. If it is a yieldable socket, then the new added yield() seems redundant.

Because according to the buffer:set() in https://luajit.org/ext_buffer.html, it supports string and cdata. Is it possible if the buffer is set to a cdata type, which is a socket?
Copy link
Contributor Author

@chronolaw chronolaw Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Currently (in kong gatway), we are using gzip/gunzip only for string data (CP/DP config), so far as I know no socket to be applied to gzip/gunzip.
chobits
chobits approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@chobits chobits left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Are there any statistics to help determine the optimal gzip-yield-size?

For example, how long does it take to compress data of size GZIP_CHUNK_SIZE * 2?
This would give us a more intuitive sense of the necessary yield intervals.

I checked the yield point, thinking it safe.
@chronolaw
Copy link
Contributor Author

chronolaw commented Jul 9, 2024 •
edited
Loading

In spec/01-unit/05-utils_spec.lua there are some unit test cases for gzip/gunzip, I modified the case "long string (> 1 buffer)", set the data length to 70k * 2, and checked the time consumed.
After multiple turns I got the average time of deflate_gzip, it is about 0.001s (1ms) or below.

@chobits , do you think this data size is a suitable number for now?
@chobits
Copy link
Contributor

chobits commented Jul 9, 2024

In spec/01-unit/05-utils_spec.lua there are some unit test cases for gzip/gunzip, I modified the case "long string (> 1 buffer)", set the data length to 70k * 2, and checked the time consumed. After multiple turns I got the average time of deflate_gzip, it is about 0.001s (1ms) or below.

@chobits , do you think this data size is a suitable number for now?

The granularity of ~1ms sounds appropriate.
@chobits chobits self-requested a review July 9, 2024 08:38
@chronolaw chronolaw requested a review from ADD-SP July 10, 2024 02:30
outsinre
outsinre reviewed Jul 12, 2024
View reviewed changes
kong/tools/gzip.lua
@@ -15,7 +16,16 @@ local GZIP_CHUNK_SIZE = 65535


local function read_input_buffer(input_buffer)
local count = 0
local yield_size = GZIP_CHUNK_SIZE * 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any consideration to yield for 2 GZIP_CHUNK_SIZE?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have enough data to support this number, so GZIP_CHUNK_SIZE * 2 is just a estimated value.
@chronolaw
Copy link
Contributor Author

@ADD-SP , do you think this PR is ready for merge?
@chronolaw chronolaw changed the title feat(tools): yield when gzip or gunzip Jul 23, 2024
@ADD-SP ADD-SP merged commit e0ad71c into master Jul 23, 2024
27 checks passed
@ADD-SP ADD-SP deleted the refactor/yield_for_gzip branch July 23, 2024 05:59
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:
chronolaw added a commit that referenced this pull request Jul 23, 2024
@chronolaw
docs(changelog): typofix for #13338
5bc9dc9
@chronolaw chronolaw mentioned this pull request Jul 23, 2024
Merged
3 tasks
ADD-SP pushed a commit that referenced this pull request Jul 23, 2024
@chronolaw
docs(changelog): typofix for #13338 (#13412)
44ecc32 
Fix typo in #13338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee size/S
6 participants
@chronolaw @chobits @team-gateway-bot @zhongweiy @outsinre @ADD-SP