Skip to content

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)

@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.

@Baoyuantop Baoyuantop removed wait for update wait for the author's response in this issue/PR user responded labels Jun 16, 2025
@Baoyuantop
Copy link
Contributor

Please fix the failed ci

@xuruidong
Copy link
Contributor Author

Please fix the failed ci

Hi @Baoyuantop ,please trigger the ci

@xuruidong
Copy link
Contributor Author

Hi @Baoyuantop , the ci is not stable, the failed test should be rerun

@Baoyuantop
Copy link
Contributor

Hi @xuruidong, there are still bad CIs that need to be fixed.

@Baoyuantop
Copy link
Contributor

Hi @xuruidong, any updates?

@xuruidong
Copy link
Contributor Author

Hi @xuruidong, any updates?

Hi @Baoyuantop ,please trigger the ci

Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 27, 2025
@Baoyuantop
Copy link
Contributor

Hi @xuruidong, are you still working on this pull request? There are some failed tests that need to be addressed.

@github-actions github-actions bot removed the stale label Sep 29, 2025
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants