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.

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

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Oct 23, 2023

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

  • 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

Full changelog

  • [Implement ...]

Issue reference

Fix #[issue number]

@chronolaw chronolaw marked this pull request as ready for review October 23, 2023 11:41
@chronolaw chronolaw changed the title refactor(pdk): serialize msg with string.buffer refactor(pdk): serialize log msg with string.buffer Oct 24, 2023
@chobits
Copy link
Contributor

chobits commented Oct 24, 2023

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 :put() in serialization function [1~n]

@chobits chobits self-requested a review October 24, 2023 07:06
@chronolaw chronolaw added the pr/discussion This PR is being debated. Probably just a few details. label Oct 24, 2023
@locao locao requested a review from zhongweiy October 24, 2023 16:38
@zhongweiy
Copy link
Contributor

Do we have any performance change data after this optimisation?

And the Jira ticket is missing in the PR message.

@chronolaw
Copy link
Contributor Author

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.

@kikito
Copy link
Member

kikito commented Oct 31, 2023

@chronolaw does this refactor provide a perf improvement?

@chronolaw
Copy link
Contributor Author

@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.
It seems that it is a little hard to test so small change of code, do we need to do this?

@zhongweiy
Copy link
Contributor

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.

@windmgc windmgc merged commit 5d2c511 into master Nov 7, 2023
29 checks passed
@windmgc windmgc deleted the refactor/serializers_with_string.buffer branch November 7, 2023 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/logs pr/discussion This PR is being debated. Probably just a few details. size/M skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants