Skip to content

perf(etcd): refactor some code to improve performance #12011

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xuruidong
Copy link
Contributor

@xuruidong xuruidong commented Feb 28, 2025

Description

Fixes # (issue)

Before:
The original code performed a cleanup of self.values and rebuilt self.values_hash by:

  • Made a full copy of self.values
  • Cleared the original and reinserted non-nil values
  • Rebuilt values_hash in a separate step

Problems:

  • Wasted memory (copied entire table)
  • Slow (processed data twice)

Now:

  1. In-place cleanup:
    • Removes nil values while keeping the same table
    • Updates values_hash at the same time
  2. Trims empty space:
    • Deletes unused slots at the end

Why better?

  • Faster: Does everything in one pass
  • Uses less memory: No extra table copies
  • Works better for big datasets

Before:
The original code performed a cleanup of self.values and rebuilt self.values_hash by:

  • Made a full copy of self.values
  • Cleared the original and reinserted non-nil values
  • Rebuilt values_hash in a separate step

Problems:

  • Wasted memory (copied entire table)
  • Slow (processed data twice)

Now:

  1. In-place cleanup:
    • Removes nil values while keeping the same table
    • Updates values_hash at the same time
  2. Trims empty space:
    • Deletes unused slots at the end

Why better?

  • Faster: Does everything in one pass
  • Uses less memory: No extra table copies
  • Works better for big datasets

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Signed-off-by: xuruidong <xuruidong@gmail.com>
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. performance generate flamegraph for the current PR labels Feb 28, 2025
@juzhiyuan
Copy link
Member

@xuruidong Just approved to run the CI

@xuruidong
Copy link
Contributor Author

@xuruidong Just approved to run the CI

please run the CI again @juzhiyuan

@juzhiyuan
Copy link
Member

@xuruidong Just approved to run the CI

please run the CI again @juzhiyuan

Hi @xuruidong, I couldn't find the Re-run button. Can you confirm the failed test cases?

image

@xuruidong
Copy link
Contributor Author

I don't think the CI failure is related to the code changes.

@Baoyuantop
Copy link
Contributor

Hi @xuruidong, can you add some descriptive information for PR?

@Baoyuantop
Copy link
Contributor

Hi @xuruidong, please add a PR description so that others can review it better ~

@Baoyuantop Baoyuantop added the wait for update wait for the author's response in this issue/PR label Apr 29, 2025
@xuruidong
Copy link
Contributor Author

Hi @xuruidong, please add a PR description so that others can review it better ~

Hi @Baoyuantop , the PR description has been added, please help review it. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance generate flamegraph for the current PR size:S This PR changes 10-29 lines, ignoring generated files. user responded wait for update wait for the author's response in this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants