-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
refactor(pdk): serialize log msg with string.buffer #11811
Conversation
I have reviewed the source code and confirmed that the new refactoring logic aligns with the original. However I'm uncertain if this PR can improve the perfomance of string serialization. Its beneficial to run some simple tests on random string sets and compare the time consumption. I noticed that the new refactoring logic uses more efficient string.buf library, but it also has introduced a higher number of function calls of |
Do we have any performance change data after this optimisation? And the Jira ticket is missing in the PR message. |
Not all PR need a Jira ticket, especially for some small changes. |
@chronolaw does this refactor provide a perf improvement? |
I have no proper data to prove the improvement of performance, but I am sure it will be helpful for perf. |
I checked there are already a bunch of similar changes done before: https://github.com/Kong/kong-ee/pulls?q=is%3Apr+is%3Aclosed+author%3Achronolaw+string Also there is similar discussion like #11304 (comment), which provides performance improvement like 5x time better when replacing table with string buffer in similar usage. Based on this, I feel it is OK to do similar replacement. @chronolaw Sorry for keeping the review process long and I'm a little bit conservative before checking the previous similar changes. |
Summary
Use
string.buffer
to optimize string operation.Here I simply replace table.insert and table.concat, but not sure the
serializers[n]
's effect, so just keep them.Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdFull changelog
Issue reference
Fix #[issue number]