Skip to content

DAOS-17550 object: refine dc_array_set_size processing #16404

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
Open
wants to merge 2 commits into
base: release/2.6
Choose a base branch
from
Open

DAOS-17550 object: refine dc_array_set_size processing #16404

wants to merge 2 commits into from

Conversation

liuxuezhao
Copy link
Contributor

  1. use memcmp rather than strncmp in merge_key strncmp can cause some keys be incorrectly ignored
  2. refine list dkey by providing more memory to save RPC round-trip.

Steps for the author:

  • Commit message follows the guidelines.
  • Appropriate Features or Test-tag pragmas were used.
  • Appropriate Functional Test Stages were run.
  • At least two positive code reviews including at least one code owner from each category referenced in the PR.
  • Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

  • Gatekeeper requested (daos-gatekeeper added as a reviewer).

@liuxuezhao liuxuezhao requested review from a team as code owners May 18, 2025 07:03
@github-actions GitHub Actions
Copy link

Ticket title is 'truncating an EC file to 0 does not work on the first try'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-17550

@liuxuezhao liuxuezhao force-pushed the lxz/key_cmp_fix_26 branch from a1cdd6f to 8d8f3ec Compare May 18, 2025 14:44
mchaarawi
mchaarawi approved these changes May 18, 2025
View reviewed changes
src/object/cli_obj.c
@@ -3573,7 +3573,7 @@ merge_key(struct dc_object *obj, d_list_t *head, char *key, int key_size)

d_list_for_each_entry(key_one, head, key_list) {
if (key_size == key_one->key.iov_len &&
strncmp(key_one->key.iov_buf, key, key_size) == 0) {
memcmp(key_one->key.iov_buf, key, key_size) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

yes that makes sense because integer dkeys are not strings

@mchaarawi
Copy link
Contributor

i have not tested on aurora yet, but will do soon

mchaarawi
mchaarawi requested changes May 18, 2025
View reviewed changes
Copy link
Contributor

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

im going to revert my +1 because the performance of the patch on aurora increases by 5x.

i just tested the memcmp fix, and that works as expected.
but the "improvement" in the array code made truncate worse my 5x:
With 4:26.38 minutes
Without 0:50.03 minutes

let's remove the other changes from the PR and just keep the memcmp fix please

@liuxuezhao liuxuezhao force-pushed the lxz/key_cmp_fix_26 branch from 8d8f3ec to 353b125 Compare May 19, 2025 03:43
@liuxuezhao
Copy link
Contributor Author

liuxuezhao commented May 19, 2025 •
edited
Loading

im going to revert my +1 because the performance of the patch on aurora increases by 5x.

i just tested the memcmp fix, and that works as expected. but the "improvement" in the array code made truncate worse my 5x: With 4:26.38 minutes Without 0:50.03 minutes

let's remove the other changes from the PR and just keep the memcmp fix please

could you please test again with the refreshed PR? the original code only list 5 dkeys every time it looks not very good, you may tune the number of "ENUM_DESC_NR_MAX" and the nr/buf_size in sub_anchors_prep() to see what is better on aurora. thanks

@liuxuezhao liuxuezhao requested a review from mchaarawi May 19, 2025 04:04
@liuxuezhao liuxuezhao force-pushed the lxz/key_cmp_fix_26 branch from 353b125 to e59cc18 Compare May 19, 2025 08:34
@liuxuezhao
DAOS-17550 object: refine dc_array_set_size processing
ec06fbf 
1. use memcmp rather than strncmp in merge_key
   strncmp can cause some keys be incorrectly ignored
2. refine list dkey by providing more memory to save RPC round-trip.

Signed-off-by: Xuezhao Liu <[email protected]>
@liuxuezhao liuxuezhao force-pushed the lxz/key_cmp_fix_26 branch from e59cc18 to ec06fbf Compare May 19, 2025 08:40
mchaarawi
mchaarawi approved these changes May 19, 2025
View reviewed changes
Copy link
Contributor

@mchaarawi mchaarawi left a comment

Choose a reason for hiding this comment

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

verified functional and performance is back to normal

@daosbuild3
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16404/2/execution/node/1372/log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchaarawi mchaarawi mchaarawi approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@liuxuezhao @mchaarawi @daosbuild3